136907 2014-09-17 21:43:44 -0700 Text laid out with the SVG -> OTF font converter does not have the same metrics as with the SVG font code path 2014-09-18 15:54:10 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mmaxfield mmaxfield darin dino jonlee mitz simon.fraser thorton oldest_to_newest 1035835 0 mmaxfield 2014-09-17 21:43:44 -0700 Text laid out with the SVG -> OTF font converter does not have the same metrics as with the SVG font code path 1035836 1 238287 mmaxfield 2014-09-17 21:50:14 -0700 Created attachment 238287 Patch 1035938 2 238287 darin 2014-09-18 09:32:26 -0700 Comment on attachment 238287 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238287&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:221 > + // If the font parser sees an ascent or descent of 0, it thinks that something > + // bad happened and takes a different path. We want it to believe us. > + if (!ascent) > + ascent = 1; > + if (!descent) > + descent = 1; Nothing in this comment makes it clear why “1” is a suitable value to use instead of 0. Also “want it to believe us” is a vague comment. It would be more useful to say something brief and clear about how specifying a 1 FUnit value gives us the behavior we want. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:226 > + // WebKit's SVG codepath hardcodes the line gap to be 1/10th of the font size (see SVGFontData.cpp). We should match that. This comment is missing the “why”. Why should we match that? > Source/WebCore/svg/SVGToOTFFontConversion.cpp:683 > + if (!advanceAttribute.isEmpty()) { Why the special case for the empty string? The code would do the right thing without a special case and I’d like this code better without a local variable for advanceAttribute. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:702 > auto& advanceAttribute = glyph.fastGetAttribute(SVGNames::horiz_adv_xAttr); This code has the same issues as the copied/pasted version of it that I commented on above. 1036006 3 238287 mmaxfield 2014-09-18 15:50:10 -0700 Comment on attachment 238287 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238287&action=review >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:221 >> + descent = 1; > > Nothing in this comment makes it clear why “1” is a suitable value to use instead of 0. Also “want it to believe us” is a vague comment. It would be more useful to say something brief and clear about how specifying a 1 FUnit value gives us the behavior we want. Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:226 >> + // WebKit's SVG codepath hardcodes the line gap to be 1/10th of the font size (see SVGFontData.cpp). We should match that. > > This comment is missing the “why”. Why should we match that? Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:683 >> + if (!advanceAttribute.isEmpty()) { > > Why the special case for the empty string? The code would do the right thing without a special case and I’d like this code better without a local variable for advanceAttribute. Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:702 >> auto& advanceAttribute = glyph.fastGetAttribute(SVGNames::horiz_adv_xAttr); > > This code has the same issues as the copied/pasted version of it that I commented on above. Done. 1036008 4 mmaxfield 2014-09-18 15:54:10 -0700 http://trac.webkit.org/changeset/173739 238287 2014-09-17 21:50:14 -0700 2014-09-18 15:50:10 -0700 Patch bug-136907-20140917215011.patch text/plain 4223 mmaxfield U3VidmVyc2lvbiBSZXZpc2lvbjogMTczNjk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMGYzZjJmMTIwZjM3ZjJk MzNkZTg0ZWIwNDE4MWVmMjIzNTQ2YWRjNS4uNTlhNmRhMGMyYjQxZWJkYjA2MzgwYmM0ZThlMzJh ZGRkMDY3YjE4NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI3IEBACisyMDE0LTA5LTE3ICBNeWxl cyBDLiBNYXhmaWVsZCAgPG1tYXhmaWVsZEBhcHBsZS5jb20+CisKKyAgICAgICAgVGV4dCBsYWlk IG91dCB3aXRoIHRoZSBTVkcgLT4gT1RGIGZvbnQgY29udmVydGVyIGRvZXMgbm90IGhhdmUgdGhl IHNhbWUgbWV0cmljcyBhcyB3aXRoIHRoZSBTVkcgZm9udCBjb2RlIHBhdGgKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNjkwNworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZXJlIGFyZSB0aHJlZSB0aGlu Z3MgdGhhdCBhcmUgY2F1c2luZyB0ZXh0IGxhaWQgb3V0IHdpdGggdGhlIFNWRyAtPiBPVEYgZm9u dCBjb252ZXJ0ZXIgdG8gbm90IGhhdmUgbWV0cmljcyB0aGF0CisgICAgICAgIG1lYXN1cmUgb3Vy IGV4aXN0aW5nIFNWRyBjb2RlcGF0aC4gVGhleSBhcmU6CisKKyAgICAgICAgMS4gQ3JlYXRpbmcg YSBmb250IHdpdGggYSAwIGFzY2VudCBvciBkZXNjZW50IG1ha2VzIE9TIFggdGhpbmsgdGhhdCBz b21ldGhpbmcgaXMgd3Jvbmcgd2l0aCB0aGUgZm9udCwgYW5kIHRha2UKKyAgICAgICAgYSBkaWZm ZXJlbnQgY29kZXBhdGggd2hlbiB0cnlpbmcgdG8gcGFyc2UgYXNjZW50IGFuZCBkZXNjZW50IGlu Zm9ybWF0aW9uLiBUaGlzIHBhdGNoIGNoZWNrcyBmb3IgdGhpcyBjb25kaXRpb24KKyAgICAgICAg YW5kIHNldHMgdGhlIGFzY2VudC9kZXNjZW50IHRvIDEgRlVuaXQgaW5zdGVhZCAod2hpY2ggaXMg Z2VuZXJhbGx5IG11Y2ggc21hbGxlciB0aGFuIGEgcGl4ZWwpLgorICAgICAgICAyLiBPdXIgU1ZH IGZvbnQgY29kZXBhdGggaGFyZGNvZGVzIGEgbGluZSBnYXAgb2YgMS8xMHRoIG9mIHRoZSBmb250 IHNpemUgZm9yIGV2ZXJ5IGZvbnQuIFRoaXMgcGF0Y2ggbWFrZXMgdGhlCisgICAgICAgIGZvbnQg Y29udmVydGVyIG9iZXkgdGhpcy4KKyAgICAgICAgMy4gVGhlIGNvbnZlcnRlciB3YXMgbm90IGFs bG93aW5nIGZvciBkZWZhdWx0IGdseXBoIGFkdmFuY2VzIGFzIHBlciB0aGUgU1ZHIGZvbnQgc3Bl Y2lmaWNhdGlvbi4gVGhpcyBwYXRjaAorICAgICAgICBkb2VzIHNvLgorCisgICAgICAgIE5vIG5l dyB0ZXN0cyB5ZXQsIGJ1dCB0aGV5IHdpbGwgY29tZSBzb29uISBJIHByb21pc2UhCisKKyAgICAg ICAgKiBzdmcvU1ZHVG9PVEZGb250Q29udmVyc2lvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpT VkdUb09URkZvbnRDb252ZXJ0ZXI6OmFwcGVuZEhIRUFUYWJsZSk6CisgICAgICAgIChXZWJDb3Jl OjpTVkdUb09URkZvbnRDb252ZXJ0ZXI6OlNWR1RvT1RGRm9udENvbnZlcnRlcik6CisKIDIwMTQt MDktMTcgIEdhdmluIEJhcnJhY2xvdWdoICA8YmFyYWNsb3VnaEBhcHBsZS5jb20+CiAKICAgICAg ICAgQXdheXMgaGF2ZSBhIFBhZ2VUaHJvdHRsZXIgKHNvbWV0aW1lcyBoYXZlIGEgVXNlckFjdGl2 aXR5OjpJbXBsKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvc3ZnL1NWR1RvT1RGRm9udENv bnZlcnNpb24uY3BwIGIvU291cmNlL1dlYkNvcmUvc3ZnL1NWR1RvT1RGRm9udENvbnZlcnNpb24u Y3BwCmluZGV4IGIwMWQ4Y2ExYWFiY2FhYzUyMzI0MmVhYmVjODk4Y2M1YTFlZDA4NjcuLmU5N2Zm ZjViZGIwN2E5OGVhYTA3ODAwZTY1NWE2MDliY2EyNzYwNzYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL3N2Zy9TVkdUb09URkZvbnRDb252ZXJzaW9uLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9zdmcvU1ZHVG9PVEZGb250Q29udmVyc2lvbi5jcHAKQEAgLTIxMywxMCArMjEzLDE4IEBAIHZv aWQgU1ZHVG9PVEZGb250Q29udmVydGVyOjphcHBlbmRISEVBVGFibGUoVmVjdG9yPGNoYXI+JiBy ZXN1bHQpIGNvbnN0CiAgICAgICAgIGRlc2NlbnQgPSBtX2ZvbnRGYWNlRWxlbWVudC0+ZGVzY2Vu dCgpOwogICAgIH0KIAorICAgIC8vIElmIHRoZSBmb250IHBhcnNlciBzZWVzIGFuIGFzY2VudCBv ciBkZXNjZW50IG9mIDAsIGl0IHRoaW5rcyB0aGF0IHNvbWV0aGluZworICAgIC8vIGJhZCBoYXBw ZW5lZCBhbmQgdGFrZXMgYSBkaWZmZXJlbnQgcGF0aC4gV2Ugd2FudCBpdCB0byBiZWxpZXZlIHVz LgorICAgIGlmICghYXNjZW50KQorICAgICAgICBhc2NlbnQgPSAxOworICAgIGlmICghZGVzY2Vu dCkKKyAgICAgICAgZGVzY2VudCA9IDE7CisKICAgICB3cml0ZTMyKHJlc3VsdCwgMHgwMDAxMDAw MCk7IC8vIFZlcnNpb24KICAgICB3cml0ZTE2KHJlc3VsdCwgYXNjZW50KTsKICAgICB3cml0ZTE2 KHJlc3VsdCwgZGVzY2VudCk7Ci0gICAgd3JpdGUxNihyZXN1bHQsIDApOyAvLyBMaW5lIGdhcAor ICAgIC8vIFdlYktpdCdzIFNWRyBjb2RlcGF0aCBoYXJkY29kZXMgdGhlIGxpbmUgZ2FwIHRvIGJl IDEvMTB0aCBvZiB0aGUgZm9udCBzaXplIChzZWUgU1ZHRm9udERhdGEuY3BwKS4gV2Ugc2hvdWxk IG1hdGNoIHRoYXQuCisgICAgd3JpdGUxNihyZXN1bHQsIHVuaXRzUGVyRW0gLyAxMCk7IC8vIExp bmUgZ2FwCiAgICAgd3JpdGUxNihyZXN1bHQsIGNsYW1wVG88dWludDE2X3QsIGZsb2F0PihtX2Fk dmFuY2VXaWR0aE1heCkpOwogICAgIHdyaXRlMTYocmVzdWx0LCBjbGFtcFRvPGludDE2X3QsIGZs b2F0PihtX2JvdW5kaW5nQm94LngoKSkpOyAvLyBNaW5pbXVtIGxlZnQgc2lkZSBiZWFyaW5nCiAg ICAgd3JpdGUxNihyZXN1bHQsIGNsYW1wVG88aW50MTZfdCwgZmxvYXQ+KG1fbWluUmlnaHRTaWRl QmVhcmluZykpOyAvLyBNaW5pbXVtIHJpZ2h0IHNpZGUgYmVhcmluZwpAQCAtNjcwLDYgKzY3OCwx NyBAQCBTVkdUb09URkZvbnRDb252ZXJ0ZXI6OlNWR1RvT1RGRm9udENvbnZlcnRlcihjb25zdCBT VkdGb250RWxlbWVudCYgZm9udEVsZW1lbnQpCiAgICAgLCBtX3dlaWdodCg1KQogICAgICwgbV9p dGFsaWMoZmFsc2UpCiB7CisgICAgZmxvYXQgZGVmYXVsdEFkdmFuY2UgPSAwOworICAgIGF1dG8m IGFkdmFuY2VBdHRyaWJ1dGUgPSBmb250RWxlbWVudC5mYXN0R2V0QXR0cmlidXRlKFNWR05hbWVz Ojpob3Jpel9hZHZfeEF0dHIpOworICAgIGlmICghYWR2YW5jZUF0dHJpYnV0ZS5pc0VtcHR5KCkp IHsKKyAgICAgICAgYm9vbCBvayA9IHRydWU7CisgICAgICAgIGZsb2F0IGFkdmFuY2UgPSBhZHZh bmNlQXR0cmlidXRlLnRvRmxvYXQoJm9rKTsKKyAgICAgICAgaWYgKG9rKSB7CisgICAgICAgICAg ICBkZWZhdWx0QWR2YW5jZSA9IGFkdmFuY2U7CisgICAgICAgICAgICBtX2FkdmFuY2VXaWR0aE1h eCA9IHN0ZDo6bWF4KG1fYWR2YW5jZVdpZHRoTWF4LCBhZHZhbmNlKTsKKyAgICAgICAgfQorICAg IH0KKwogICAgIC8vIEZJWE1FOiBVc2UgdGhlIG1pc3NpbmdHbHlwaCBpbmZvCiAgICAgVmVjdG9y PGNoYXIsIDE+IG5vdGRlZkNoYXJTdHJpbmc7CiAgICAgbm90ZGVmQ2hhclN0cmluZy5hcHBlbmQo ZW5kQ2hhcik7CkBAIC02NzksNyArNjk4LDcgQEAgU1ZHVG9PVEZGb250Q29udmVydGVyOjpTVkdU b09URkZvbnRDb252ZXJ0ZXIoY29uc3QgU1ZHRm9udEVsZW1lbnQmIGZvbnRFbGVtZW50KQogICAg ICAgICBhdXRvJiB1bmljb2RlQXR0cmlidXRlID0gZ2x5cGguZmFzdEdldEF0dHJpYnV0ZShTVkdO YW1lczo6dW5pY29kZUF0dHIpOwogICAgICAgICAvLyBPbmx5IHN1cHBvcnQgQmFzaWMgTXVsdGls aW5ndWFsIFBsYW5lIHcvbyBsaWdhdHVyZXMgZm9yIG5vdwogICAgICAgICBpZiAodW5pY29kZUF0 dHJpYnV0ZS5sZW5ndGgoKSA9PSAxKSB7Ci0gICAgICAgICAgICBmbG9hdCBlZmZlY3RpdmVBZHZh bmNlID0gMDsKKyAgICAgICAgICAgIGZsb2F0IGVmZmVjdGl2ZUFkdmFuY2UgPSBkZWZhdWx0QWR2 YW5jZTsKICAgICAgICAgICAgIGF1dG8mIGFkdmFuY2VBdHRyaWJ1dGUgPSBnbHlwaC5mYXN0R2V0 QXR0cmlidXRlKFNWR05hbWVzOjpob3Jpel9hZHZfeEF0dHIpOwogICAgICAgICAgICAgaWYgKCFh ZHZhbmNlQXR0cmlidXRlLmlzRW1wdHkoKSkgewogICAgICAgICAgICAgICAgIGJvb2wgb2sgPSB0 cnVlOwo=