215302 2020-08-07 16:40:12 -0700 Update OriginalAdvancesForCharacterTreatedAsSpace to work correctly in the presence of inserted or removed glyphs 2020-08-09 00:37:21 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 215051 206208 1 mmaxfield mmaxfield darin dino jonlee koivisto simon.fraser thorton webkit-bug-importer zalan oldest_to_newest 1678817 0 mmaxfield 2020-08-07 16:40:12 -0700 Update OriginalAdvancesForCharacterTreatedAsSpace to work properly after r265241 1678821 1 406228 mmaxfield 2020-08-07 16:45:26 -0700 Created attachment 406228 Patch 1678911 2 406228 darin 2020-08-08 11:36:24 -0700 Comment on attachment 406228 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406228&action=review > Source/WebCore/ChangeLog:3 > + Update OriginalAdvancesForCharacterTreatedAsSpace to work properly after r265241 "work properly" seems ambiguous Do you mean "work without depending on something that’s been true in practice, but not guaranteed", "work more efficiently", "work in a way that’s more maintainable", or "work correctly"? I’m assuming it’s the first. Can we find a slightly more precise way of saying that in the title, not just the change log comment? > Source/WebCore/ChangeLog:11 > + However, this is wrong, becuase shaping can insert or delete glyphs. Instead, now that we have "becuase" > Source/WebCore/platform/graphics/WidthIterator.cpp:61 > public: struct doesn’t need "public" > Source/WebCore/platform/graphics/WidthIterator.cpp:73 > + bool characterIsSpace = false; > + float advanceBeforeCharacter = 0; > + float advanceAtCharacter = 0; In WebKit we’ve been using { false } instead of = false in cases like this. > Source/WebCore/platform/graphics/WidthIterator.h:38 > +typedef HashMap<unsigned, OriginalAdvancesForCharacterTreatedAsSpace, DefaultHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> CharactersTreatedAsSpace; I think a sorted vector and std::binary_search might be better than a HashMap for this. New code should use "using" instead of "typedef". Switch this since you are touching it, please. What guarantees we never use 0xFFFFFFFE or 0xFFFFFFFF? If we need to stick with a HasMap, can we make this even more efficient by using 16 bits? 1678912 3 406228 darin 2020-08-08 11:36:53 -0700 Comment on attachment 406228 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406228&action=review >> Source/WebCore/platform/graphics/WidthIterator.h:38 >> +typedef HashMap<unsigned, OriginalAdvancesForCharacterTreatedAsSpace, DefaultHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> CharactersTreatedAsSpace; > > I think a sorted vector and std::binary_search might be better than a HashMap for this. > > New code should use "using" instead of "typedef". Switch this since you are touching it, please. > > What guarantees we never use 0xFFFFFFFE or 0xFFFFFFFF? If we need to stick with a HasMap, can we make this even more efficient by using 16 bits? I think I mean std::lower_bound. 1678951 4 mmaxfield 2020-08-09 00:36:29 -0700 Committed r265415: <https://trac.webkit.org/changeset/265415> 1678952 5 webkit-bug-importer 2020-08-09 00:37:21 -0700 <rdar://problem/66742090> 406228 2020-08-07 16:45:26 -0700 2020-08-08 11:36:24 -0700 Patch bug-215302-20200807164526.patch text/plain 5918 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMjY1MzkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjM0ZWFmZGRmMTRjODhj ZDE3ZWVhM2RjZDA1MTY0ODQxNWM1NThjYy4uODRjZjVhNTQ3Y2YxMWYyNWVhZWZmYmMwN2EyY2Mz ZDcyYmFmNGVkNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDIwLTA4LTA3ICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICAgVXBkYXRlIE9y aWdpbmFsQWR2YW5jZXNGb3JDaGFyYWN0ZXJUcmVhdGVkQXNTcGFjZSB0byB3b3JrIHByb3Blcmx5 IGFmdGVyIHIyNjUyNDEKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTIxNTMwMgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIE9yaWdpbmFsQWR2YW5jZXNGb3JDaGFyYWN0ZXJUcmVhdGVkQXNTcGFjZSBpcyB0cnlp bmcgdG8gbWFrZSBzdXJlIHRoYXQgc2hhcGluZyBkb2Vzbid0IGNhdXNlCisgICAgICAgIHNwYWNl cyB0byBnZXQgd2lkZXIgb3IgdGhpbm5lci4gSG93ZXZlciwgdGhlIHdheSBpdCB3YXMgZG9pbmcg dGhhdCBpcywgZm9yIGFsbCB0aGUgc3BhY2UKKyAgICAgICAgY2hhcmFjdGVycywgb3ZlcndyaXRl IHRoYXQgZ2x5cGggaW5kZXgncyBhZHZhbmNlIGFmdGVyIHNoYXBpbmcgdG8gYmUgd2hhdCBpdCB3 YXMgYmVmb3JlIHNoYXBpbmcuCisgICAgICAgIEhvd2V2ZXIsIHRoaXMgaXMgd3JvbmcsIGJlY3Vh c2Ugc2hhcGluZyBjYW4gaW5zZXJ0IG9yIGRlbGV0ZSBnbHlwaHMuIEluc3RlYWQsIG5vdyB0aGF0 IHdlIGhhdmUKKyAgICAgICAgZXhwbGljaXQgc3RyaW5nIGluZGljZXMgZm9yIGVhY2ggZ2x5cGgs IHdlIGNhbiB1c2UgdGhvc2UgdG8gZGV0ZXJtaW5lIHdoaWNoIGdseXBocyBjb21lIGZyb20KKyAg ICAgICAgc3BhY2UgY2hhcmFjdGVycy4gVGhlc2UgZ2x5cGhzIGFyZSB0aGUgb25lcyB3aGljaCBz aG91bGQgYmUgb3ZlcndyaXR0ZW4uCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9XaWR0 aEl0ZXJhdG9yLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OldpZHRoSXRlcmF0b3I6OmFwcGx5Rm9u dFRyYW5zZm9ybXMpOgorICAgICAgICAoV2ViQ29yZTo6V2lkdGhJdGVyYXRvcjo6YWR2YW5jZUlu dGVybmFsKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9XaWR0aEl0ZXJhdG9yLmg6CisK IDIwMjAtMDgtMDcgIFJvYiBCdWlzICA8cmJ1aXNAaWdhbGlhLmNvbT4KIAogICAgICAgICBJbXBs ZW1lbnQgUGVyZm9tYW5jZU9ic2VydmVySW5pdC5idWZmZXJlZApkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvV2lkdGhJdGVyYXRvci5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9XaWR0aEl0ZXJhdG9yLmNwcAppbmRleCA1NDU1YmJkZjcw NTc0ZmQ1NTcyZTZjYzYzNjM4OGUzYTJhY2MyMTNhLi5kZDRmY2ExYzNlMzM3NjkzNzVkMGE5NWJj NWI2MmM1YTE0ZGYyYzhlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9XaWR0aEl0ZXJhdG9yLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9XaWR0aEl0ZXJhdG9yLmNwcApAQCAtNTksNiArNTksOCBAQCBXaWR0aEl0ZXJhdG9yOjpX aWR0aEl0ZXJhdG9yKGNvbnN0IEZvbnRDYXNjYWRlJiBmb250LCBjb25zdCBUZXh0UnVuJiBydW4s IEhhc2hTZQogCiBzdHJ1Y3QgT3JpZ2luYWxBZHZhbmNlc0ZvckNoYXJhY3RlclRyZWF0ZWRBc1Nw YWNlIHsKIHB1YmxpYzoKKyAgICBPcmlnaW5hbEFkdmFuY2VzRm9yQ2hhcmFjdGVyVHJlYXRlZEFz U3BhY2UoKSA9IGRlZmF1bHQ7CisKICAgICBleHBsaWNpdCBPcmlnaW5hbEFkdmFuY2VzRm9yQ2hh cmFjdGVyVHJlYXRlZEFzU3BhY2UoYm9vbCBpc1NwYWNlLCBmbG9hdCBhZHZhbmNlQmVmb3JlLCBm bG9hdCBhZHZhbmNlQXQpCiAgICAgICAgIDogY2hhcmFjdGVySXNTcGFjZShpc1NwYWNlKQogICAg ICAgICAsIGFkdmFuY2VCZWZvcmVDaGFyYWN0ZXIoYWR2YW5jZUJlZm9yZSkKQEAgLTY2LDkgKzY4 LDkgQEAgcHVibGljOgogICAgIHsKICAgICB9CiAKLSAgICBib29sIGNoYXJhY3RlcklzU3BhY2U7 Ci0gICAgZmxvYXQgYWR2YW5jZUJlZm9yZUNoYXJhY3RlcjsKLSAgICBmbG9hdCBhZHZhbmNlQXRD aGFyYWN0ZXI7CisgICAgYm9vbCBjaGFyYWN0ZXJJc1NwYWNlID0gZmFsc2U7CisgICAgZmxvYXQg YWR2YW5jZUJlZm9yZUNoYXJhY3RlciA9IDA7CisgICAgZmxvYXQgYWR2YW5jZUF0Q2hhcmFjdGVy ID0gMDsKIH07CiAKIHN0YXRpYyBpbmxpbmUgYm9vbCBpc1NvZnRCYW5rRW1vamkoVUNoYXIzMiBj b2RlcG9pbnQpCkBAIC0xMTYsMTggKzExOCwxNSBAQCBpbmxpbmUgZmxvYXQgV2lkdGhJdGVyYXRv cjo6YXBwbHlGb250VHJhbnNmb3JtcyhHbHlwaEJ1ZmZlciogZ2x5cGhCdWZmZXIsIGJvb2wgbAog ICAgIGlmICghbHRyKQogICAgICAgICBnbHlwaEJ1ZmZlci0+cmV2ZXJzZShsYXN0R2x5cGhDb3Vu dCwgZ2x5cGhCdWZmZXJTaXplIC0gbGFzdEdseXBoQ291bnQpOwogCi0gICAgLy8gaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwNjIwODogVGhpcyBpcyB0b3RhbGx5LCAx MDAlLCBmdXJpb3VzbHksIHV0dGVybHksIGZydXN0cmF0aW5nbHkgYm9ndXMuCi0gICAgLy8gVGhl cmUgaXMgYWJzb2x1dGVseSBubyBndWFyYW50ZWUgdGhhdCBnbHlwaCBpbmRpY2VzIGJlZm9yZSBz aGFwaW5nIGhhdmUgYW55IHJlbGF0aW9uIGF0IGFsbCB3aXRoIGdseXBoIGluZGljZXMgYWZ0ZXIg c2hhcGluZy4KLSAgICAvLyBPbmUgb2YgdGhlIGZ1bmRhbWVudGFsIHRoaW5ncyB0aGF0IHNoYXBp bmcgZG9lcyBpcyBpbnNlcnQgZ2x5cGggYWxsIG92ZXIgdGhlIHBsYWNlLgotICAgIGZvciAoc2l6 ZV90IGkgPSAwOyBpIDwgY2hhcmFjdGVyc1RyZWF0ZWRBc1NwYWNlLnNpemUoKTsgKytpKSB7Ci0g ICAgICAgIGF1dG8gc3BhY2VPZmZzZXQgPSBjaGFyYWN0ZXJzVHJlYXRlZEFzU3BhY2VbaV0uZmly c3Q7Ci0gICAgICAgIC8vIFNoYXBpbmcgbWF5IGhhdmUgZGVsZXRlZCB0aGUgZ2x5cGguCi0gICAg ICAgIGlmIChzcGFjZU9mZnNldCA+PSBnbHlwaEJ1ZmZlclNpemUpCisgICAgZm9yICh1bnNpZ25l ZCBpID0gbGFzdEdseXBoQ291bnQ7IGkgPCBnbHlwaEJ1ZmZlclNpemU7ICsraSkgeworICAgICAg ICBhdXRvIGNoYXJhY3RlckluZGV4ID0gZ2x5cGhCdWZmZXItPnN0cmluZ09mZnNldEF0KGkpOwor ICAgICAgICBhdXRvIGl0ZXJhdG9yID0gY2hhcmFjdGVyc1RyZWF0ZWRBc1NwYWNlLmZpbmQoY2hh cmFjdGVySW5kZXgpOworICAgICAgICBpZiAoaXRlcmF0b3IgPT0gY2hhcmFjdGVyc1RyZWF0ZWRB c1NwYWNlLmVuZCgpKQogICAgICAgICAgICAgY29udGludWU7Ci0gICAgICAgIGNvbnN0IE9yaWdp bmFsQWR2YW5jZXNGb3JDaGFyYWN0ZXJUcmVhdGVkQXNTcGFjZSYgb3JpZ2luYWxBZHZhbmNlcyA9 IGNoYXJhY3RlcnNUcmVhdGVkQXNTcGFjZVtpXS5zZWNvbmQ7Ci0gICAgICAgIGlmIChzcGFjZU9m ZnNldCAmJiAhb3JpZ2luYWxBZHZhbmNlcy5jaGFyYWN0ZXJJc1NwYWNlKQotICAgICAgICAgICAg Z2x5cGhCdWZmZXItPmFkdmFuY2VzKHNwYWNlT2Zmc2V0IC0gMSktPnNldFdpZHRoKG9yaWdpbmFs QWR2YW5jZXMuYWR2YW5jZUJlZm9yZUNoYXJhY3Rlcik7Ci0gICAgICAgIGdseXBoQnVmZmVyLT5h ZHZhbmNlcyhzcGFjZU9mZnNldCktPnNldFdpZHRoKG9yaWdpbmFsQWR2YW5jZXMuYWR2YW5jZUF0 Q2hhcmFjdGVyKTsKKyAgICAgICAgY29uc3QgYXV0byYgb3JpZ2luYWxBZHZhbmNlcyA9IGl0ZXJh dG9yLT52YWx1ZTsKKyAgICAgICAgaWYgKGkgJiYgIW9yaWdpbmFsQWR2YW5jZXMuY2hhcmFjdGVy SXNTcGFjZSkKKyAgICAgICAgICAgIGdseXBoQnVmZmVyLT5hZHZhbmNlcyhpIC0gMSktPnNldFdp ZHRoKG9yaWdpbmFsQWR2YW5jZXMuYWR2YW5jZUJlZm9yZUNoYXJhY3Rlcik7CisgICAgICAgIGds eXBoQnVmZmVyLT5hZHZhbmNlcyhpKS0+c2V0V2lkdGgob3JpZ2luYWxBZHZhbmNlcy5hZHZhbmNl QXRDaGFyYWN0ZXIpOwogICAgIH0KICAgICBjaGFyYWN0ZXJzVHJlYXRlZEFzU3BhY2UuY2xlYXIo KTsKIApAQCAtMzE4LDggKzMxNyw5IEBAIGlubGluZSB2b2lkIFdpZHRoSXRlcmF0b3I6OmFkdmFu Y2VJbnRlcm5hbChUZXh0SXRlcmF0b3ImIHRleHRJdGVyYXRvciwgR2x5cGhCdWZmCiAKICAgICAg ICAgYXV0byB0cmFuc2Zvcm1zVHlwZSA9IHNob3VsZEFwcGx5Rm9udFRyYW5zZm9ybXMoZ2x5cGhC dWZmZXIsIGxhc3RHbHlwaENvdW50LCBwcmV2aW91c0NoYXJhY3Rlcik7CiAgICAgICAgIGlmICh0 cmFuc2Zvcm1zVHlwZSAhPSBUcmFuc2Zvcm1zVHlwZTo6Tm9uZSAmJiBnbHlwaEJ1ZmZlciAmJiBG b250Q2FzY2FkZTo6dHJlYXRBc1NwYWNlKGNoYXJhY3RlcikpIHsKLSAgICAgICAgICAgIGNoYXJh Y3RlcnNUcmVhdGVkQXNTcGFjZS5hcHBlbmQoc3RkOjptYWtlX3BhaXIoZ2x5cGhCdWZmZXItPnNp emUoKSwKLSAgICAgICAgICAgICAgICBPcmlnaW5hbEFkdmFuY2VzRm9yQ2hhcmFjdGVyVHJlYXRl ZEFzU3BhY2UoY2hhcmFjdGVyID09ICcgJywgZ2x5cGhCdWZmZXItPnNpemUoKSA/IGdseXBoQnVm ZmVyLT5hZHZhbmNlQXQoZ2x5cGhCdWZmZXItPnNpemUoKSAtIDEpLndpZHRoKCkgOiAwLCB3aWR0 aCkpKTsKKyAgICAgICAgICAgIGF1dG8gYWRkUmVzdWx0ID0gY2hhcmFjdGVyc1RyZWF0ZWRBc1Nw YWNlLnNldChjdXJyZW50Q2hhcmFjdGVySW5kZXgsCisgICAgICAgICAgICAgICAgT3JpZ2luYWxB ZHZhbmNlc0ZvckNoYXJhY3RlclRyZWF0ZWRBc1NwYWNlKGNoYXJhY3RlciA9PSAnICcsIGdseXBo QnVmZmVyLT5zaXplKCkgPyBnbHlwaEJ1ZmZlci0+YWR2YW5jZUF0KGdseXBoQnVmZmVyLT5zaXpl KCkgLSAxKS53aWR0aCgpIDogMCwgd2lkdGgpKTsKKyAgICAgICAgICAgIEFTU0VSVF9VTlVTRUQo YWRkUmVzdWx0LCBhZGRSZXN1bHQuaXNOZXdFbnRyeSk7CiAgICAgICAgIH0KIAogICAgICAgICBp ZiAobV9hY2NvdW50Rm9yR2x5cGhCb3VuZHMpIHsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL2dyYXBoaWNzL1dpZHRoSXRlcmF0b3IuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL1dpZHRoSXRlcmF0b3IuaAppbmRleCA4N2JhNTU4NWEyMDIwOTgxNDcyMWJk NTYzNmQ4MDlhZjZjZmQzZWU5Li43OTJiMDk0NjgxN2EwNzFhMDcyNmExODNmZGViNTFkOWY3MDI1 NjkxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9XaWR0aEl0 ZXJhdG9yLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvV2lkdGhJdGVy YXRvci5oCkBAIC0zNSw3ICszNSw3IEBAIGNsYXNzIFRleHRSdW47CiBzdHJ1Y3QgR2x5cGhEYXRh Owogc3RydWN0IE9yaWdpbmFsQWR2YW5jZXNGb3JDaGFyYWN0ZXJUcmVhdGVkQXNTcGFjZTsKIAot dHlwZWRlZiBWZWN0b3I8c3RkOjpwYWlyPHVuc2lnbmVkLCBPcmlnaW5hbEFkdmFuY2VzRm9yQ2hh cmFjdGVyVHJlYXRlZEFzU3BhY2U+LCA2ND4gQ2hhcmFjdGVyc1RyZWF0ZWRBc1NwYWNlOwordHlw ZWRlZiBIYXNoTWFwPHVuc2lnbmVkLCBPcmlnaW5hbEFkdmFuY2VzRm9yQ2hhcmFjdGVyVHJlYXRl ZEFzU3BhY2UsIERlZmF1bHRIYXNoPHVuc2lnbmVkPiwgV1RGOjpVbnNpZ25lZFdpdGhaZXJvS2V5 SGFzaFRyYWl0czx1bnNpZ25lZD4+IENoYXJhY3RlcnNUcmVhdGVkQXNTcGFjZTsKIAogc3RydWN0 IFdpZHRoSXRlcmF0b3IgewogICAgIFdURl9NQUtFX0ZBU1RfQUxMT0NBVEVEOwo=