Tweak and tighten SVG font converter
Created attachment 238379 [details] Patch
Myles, I made a few tweaks to the code you landed recently. Could you take a look?
Comment on attachment 238379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238379&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:325 > + char panoseBytes[10]; Maximum value of 0xFF means this should be unsigned (though if we're just going to append this later, it might not matter) > Source/WebCore/svg/SVGToOTFFontConversion.cpp:327 > + Vector<String> strings; This probably should have a better name than "strings". Perhaps panoseSegments? > Source/WebCore/svg/SVGToOTFFontConversion.cpp:330 > + for (auto& string : strings) { Similar to above (though it's definitely better than "s"). > Source/WebCore/svg/SVGToOTFFontConversion.cpp:334 > + panoseBytes[numPanoseBytes++] = value; no break here? > Source/WebCore/svg/SVGToOTFFontConversion.cpp:-374 > -static void appendCFFValidString(Vector<char>& output, const String& string) I feel that "valid" sets up a precondition to the function. Removing the word is fine, but I think that having some way of documenting the precondition would be beneficial. (The string must be valid because we're down casting the UChars to chars in the Vector::append() function) > Source/WebCore/svg/SVGToOTFFontConversion.cpp:422 > + const unsigned sizeOfTopIndex = 45 + (weight.isNull() ? 0 : 6); You want to write out empty strings into the font? I suppose philosophically if the string is present but empty in the SVG font, it makes sense for it to be present but empty in the OTF font? > Source/WebCore/svg/SVGToOTFFontConversion.cpp:-468 > - if (!weight.isEmpty()) { I wish we could collapse all of these String::isEmpty() calls into a single call and re-use the result, but I'm not sure if it would actually be beneficial. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:506 > + write16(result, m_fontElement.fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()); So much better! However, when I actually do use the missing glyph info for this, I'll want to predicate this fallback based on whether or not parsing failed, which means I'll have to reintroduce some of this deleted code soon. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:508 > + auto tableSizeOffset = result.size(); Conceptually this would be simpler if you moved this result.size() call after the line below that writes the placeholder. That way you wouldn't need the "- 2". > Source/WebCore/svg/SVGToOTFFontConversion.cpp:509 > + write16(result, 0); // Place to write table size. nit: s/Place/Placeholder/ > Source/WebCore/svg/SVGToOTFFontConversion.cpp:512 > + if (int16_t verticalOriginY = glyph->fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()) { Probably should clamp this to a the range of an int16_t. Outputting the origin regardless of parsing success seems iffy to me. This make an assumption that the toInt() behavior returns the same value as an unknown vertical origin, which might not be correct based on the underlying platform implementation. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:518 > + overwrite16(result, tableSizeOffset, (result.size() - tableSizeOffset - 2) / 4); Shouldn't we assert that result.size() - tableSizeOffset % 4 == 0? (Assuming that you move the tableSizeOffset after the table size placeholder) > Source/WebCore/svg/SVGToOTFFontConversion.cpp:523 > vector.append(-1); // 0xFF I'm surprised you didn't change this to vector.append(0xFF) > Source/WebCore/svg/SVGToOTFFontConversion.cpp:538 > // FIXME: We probably want the initial moveto to use horiz-origin-x and horiz-origin-y, unless we're vertical No period for this comment? ;-) > Source/WebCore/svg/SVGToOTFFontConversion.cpp:561 > + void writePoint(FloatPoint destination) Good observation (that this is possible)! > Source/WebCore/svg/SVGToOTFFontConversion.cpp:-599 > - // FIXME: This can be made way faster We can still make this faster by not computing deltas in writePoint() if we are handed deltas in the first place. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:627 > + // FIXME: Shouldn't this be isNull? Is there a reason to handle an empty string attribute the same as a missing attribute? I gave my argument above about writing empty strings to the font. I suppose in this case, however, an empty string would result in the same result. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:628 > // FIXME: Why is this different than what we do below when parsing fails. Good thought. It seems to me that the parsing fails code path is straight up broken right now. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:710 > if (segment == "bold") { I should probably make these a case insensitive compare > Source/WebCore/svg/SVGToOTFFontConversion.cpp:713 > + // Are we intentionally implementing a "last one wins" rule? Nope. The spec doesn't define what to do in this case. We might as well break here. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:718 > + // FIXME: Do we want to truncate, or would it be better to round? The spec only defines multiples of 100 as technically valid, so either way is probably fine. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:726 > + for (auto& segment : segments) { Seems like this could be joined up with the loop above it. I feel pretty sheepish for making two separate loops... > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:169 > + EXPECT_EQ(false, ok); There's no EXPECT_FALSE? > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:192 > + EXPECT_EQ(-2147483648, String("-2147483648").toInt(&ok)); Might want to test if the string appears to be larger than the largest int or smaller than the smallest int > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:212 > + EXPECT_EQ(true, ok); I don't know if it's out of scope, but it would be interesting to see what happens with things like unmatched surrogates or invalid UTF-16, or multi-byte code points. Also might want to test if "4x8" gets parsed to 4 or 8 (perhaps similarly with "4x8x") > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:246 > + // FIXME: This is an inconsistency with toInt, which always guarantees :( > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:260 > + EXPECT_EQ(false, ok); Might also want to test decimal places or things like -3e6 Same with larger than the largest double / smaller than the smallest double Is the assumption that toFloat() will work the same way as toDouble()?
unofficial r=me
Comment on attachment 238379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238379&action=review >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:325 >> + char panoseBytes[10]; > > Maximum value of 0xFF means this should be unsigned (though if we're just going to append this later, it might not matter) It’s true that the maximum value is 0xFF, but not true that we need to use the type "unsigned char" here because of that. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:327 >> + Vector<String> strings; > > This probably should have a better name than "strings". Perhaps panoseSegments? I’m not sure that a more specific name would be better. This is a very local variable and I don’t think they need globally unique names, but, sure, I’ll consider panoseSegments, even though it seems a bit wordy. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:334 >> + panoseBytes[numPanoseBytes++] = value; > > no break here? We definitely don’t want to break here. We need to do this 10 times, not 1 time. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-374 >> -static void appendCFFValidString(Vector<char>& output, const String& string) > > I feel that "valid" sets up a precondition to the function. Removing the word is fine, but I think that having some way of documenting the precondition would be beneficial. (The string must be valid because we're down casting the UChars to chars in the Vector::append() function) Hmm, it seems like you are saying that removing the word is not fine, even though you say “removing the word is fine”. Not sure what to do. I agree that the string being valid is a precondition of the function, but not that calling the function appendCFFValidString makes that clear. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:422 >> + const unsigned sizeOfTopIndex = 45 + (weight.isNull() ? 0 : 6); > > You want to write out empty strings into the font? I suppose philosophically if the string is present but empty in the SVG font, it makes sense for it to be present but empty in the OTF font? I have no idea if empty strings come up in practice. But, this code is to handle the case where no weight is specified, and that is handled with the null value, not empty value. Checking for empty value is not obviously bad, but it’s not really helpful either. I see no reason that an empty string would not be allowed, but also no reason that an empty string would be helpful or valuable either. Generally it’s bad coding style to check for a null value with isEmpty; it’s slower and does more work. And I don’t think we should do that unless we really think it’s a valuable thing to do that for an empty string. I believe we are doing this just because the isEmpty function is handy and also easy to confuse with isNull. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-468 >> - if (!weight.isEmpty()) { > > I wish we could collapse all of these String::isEmpty() calls into a single call and re-use the result, but I'm not sure if it would actually be beneficial. Well, for isEmpty there’s a tiny chance that would compiler to tighter code. For isNull it likely wouldn’t because it’s just a simple null pointer check, and I’m not sure checking a boolean local variable would be better. Should decide based on code readability, not performance optimization. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:506 >> + write16(result, m_fontElement.fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()); > > So much better! However, when I actually do use the missing glyph info for this, I'll want to predicate this fallback based on whether or not parsing failed, which means I'll have to reintroduce some of this deleted code soon. If you do that, you should figure out exactly what you mean by “parsing failed”. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:508 >> + auto tableSizeOffset = result.size(); > > Conceptually this would be simpler if you moved this result.size() call after the line below that writes the placeholder. That way you wouldn't need the "- 2". We use tableSizeOffset twice below. If I moved this after the line below then I’d have to use tableOffsetSize - 2 in the first place i use it. I don’t think that’s conceptually simpler. We could have two offsets, but I’m not sure that would be better. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:509 >> + write16(result, 0); // Place to write table size. > > nit: s/Place/Placeholder/ I meant “place”, not “placeholder”. I agree that the zero is a “placeholder”, but the place I am putting the zero is a “place”. So I chose this on purpose. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:512 >> + if (int16_t verticalOriginY = glyph->fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()) { > > Probably should clamp this to a the range of an int16_t. > > Outputting the origin regardless of parsing success seems iffy to me. This make an assumption that the toInt() behavior returns the same value as an unknown vertical origin, which might not be correct based on the underlying platform implementation. Why is clamping to the range of an int16_t the right thing to do? Should a large value turn into 32767? Should a small value turn into -32768? What makes that useful behavior? I agree that simply truncating into an int16_t isn’t right, but I don’t know why clamping is right. The “iffy” thing you are referring to doesn’t exist. The code does not write out an origin when parsing fails. When parsing fails we get a zero and the if statement evaluates to false. On a separate note, I don’t know what an “unknown vertical origin” is. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:518 >> + overwrite16(result, tableSizeOffset, (result.size() - tableSizeOffset - 2) / 4); > > Shouldn't we assert that result.size() - tableSizeOffset % 4 == 0? (Assuming that you move the tableSizeOffset after the table size placeholder) Sure it would be OK to add an assertion, but I don’t think the assertion is all that valuable. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:523 >> vector.append(-1); // 0xFF > > I'm surprised you didn't change this to vector.append(0xFF) Good point, I will change that. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:538 >> // FIXME: We probably want the initial moveto to use horiz-origin-x and horiz-origin-y, unless we're vertical > > No period for this comment? ;-) There were a lot of comments in this file. I didn’t get to every one! >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-599 >> - // FIXME: This can be made way faster > > We can still make this faster by not computing deltas in writePoint() if we are handed deltas in the first place. Maybe, but I’m not sure that “faster” is a meaningful issue here. I think the memory allocation in this overall operation is likely far more expensive than the floating point math. I don’t think this particular optimization opportunity is so valuable that we need to comment on it. Note also that if we are passed deltas, we still need to do math on two of them to convert them to deltas relative to each other, not deltas related to the start point. I don’t think there’s a compelling need to change this unless it’s producing incorrect results or really does measure as a significant performance cost. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:627 >> + // FIXME: Shouldn't this be isNull? Is there a reason to handle an empty string attribute the same as a missing attribute? > > I gave my argument above about writing empty strings to the font. I suppose in this case, however, an empty string would result in the same result. I am not sure it’s really a correct argument. Why is empty string handled specially, but not, say, a string containing a single space in it? >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:628 >> + // FIXME: Why is this different than what we do below when parsing fails. > > Good thought. It seems to me that the parsing fails code path is straight up broken right now. OK. I think we need unit tests for this code. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:710 >> + if (segment == "bold") { > > I should probably make these a case insensitive compare Well, maybe. All depends on what the SVG specification says. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:713 >> + // Are we intentionally implementing a "last one wins" rule? > > Nope. The spec doesn't define what to do in this case. We might as well break here. We should come up with a good rule. I suppose “first wins” is better than “last wins”. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:718 >> + // FIXME: Do we want to truncate, or would it be better to round? > > The spec only defines multiples of 100 as technically valid, so either way is probably fine. We need to define the behavior we want to implement for invalid values. We can’t write code that does three different things, so lets chose one. We could treat such values as invalid, or truncate them, or round them, or possibly do something else. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:726 >> + for (auto& segment : segments) { > > Seems like this could be joined up with the loop above it. I feel pretty sheepish for making two separate loops... Good point. I thought the loop was parsing two different strings. Note that joining the loops means not doing “break”, since we want to find both "bold" and "italic". >> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:169 >> + EXPECT_EQ(false, ok); > > There's no EXPECT_FALSE? Maybe there is, dunno. >> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:192 >> + EXPECT_EQ(-2147483648, String("-2147483648").toInt(&ok)); > > Might want to test if the string appears to be larger than the largest int or smaller than the smallest int That is tested here already. 2147483648 (just before this test) is larger than the largest and -2147483649 (just after this test) is smaller than the smallest. Unless you mean something different? >> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:212 >> + EXPECT_EQ(true, ok); > > I don't know if it's out of scope, but it would be interesting to see what happens with things like unmatched surrogates or invalid UTF-16, or multi-byte code points. > > Also might want to test if "4x8" gets parsed to 4 or 8 (perhaps similarly with "4x8x") Sure, we could add lots more tests. This just scratches the surface. >> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:260 >> + EXPECT_EQ(false, ok); > > Might also want to test decimal places or things like -3e6 > > Same with larger than the largest double / smaller than the smallest double > > Is the assumption that toFloat() will work the same way as toDouble()? Sure we can add more tests. I didn’t write tests for toFloat yet. It’s trickier because there is no toFloat function; it just calls toDouble and then casts to a float. That means it’s going to do the wrong things for numbers that fit in a double but not in a float.
Comment on attachment 238379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238379&action=review >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:325 >>> + char panoseBytes[10]; >> >> Maximum value of 0xFF means this should be unsigned (though if we're just going to append this later, it might not matter) > > It’s true that the maximum value is 0xFF, but not true that we need to use the type "unsigned char" here because of that. Okay. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:334 >>> + panoseBytes[numPanoseBytes++] = value; >> >> no break here? > > We definitely don’t want to break here. We need to do this 10 times, not 1 time. Whoops; please disregard this comment. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-374 >>> -static void appendCFFValidString(Vector<char>& output, const String& string) >> >> I feel that "valid" sets up a precondition to the function. Removing the word is fine, but I think that having some way of documenting the precondition would be beneficial. (The string must be valid because we're down casting the UChars to chars in the Vector::append() function) > > Hmm, it seems like you are saying that removing the word is not fine, even though you say “removing the word is fine”. Not sure what to do. I agree that the string being valid is a precondition of the function, but not that calling the function appendCFFValidString makes that clear. I meant to say that it would be beneficial to document that the input to this function should be valid, regardless of what the mechanism for that documentation is. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:422 >>> + const unsigned sizeOfTopIndex = 45 + (weight.isNull() ? 0 : 6); >> >> You want to write out empty strings into the font? I suppose philosophically if the string is present but empty in the SVG font, it makes sense for it to be present but empty in the OTF font? > > I have no idea if empty strings come up in practice. But, this code is to handle the case where no weight is specified, and that is handled with the null value, not empty value. Checking for empty value is not obviously bad, but it’s not really helpful either. I see no reason that an empty string would not be allowed, but also no reason that an empty string would be helpful or valuable either. > > Generally it’s bad coding style to check for a null value with isEmpty; it’s slower and does more work. And I don’t think we should do that unless we really think it’s a valuable thing to do that for an empty string. I believe we are doing this just because the isEmpty function is handy and also easy to confuse with isNull. Originally, I was thinking that specifying 'font-weight=""' should be treated the same as not having specified the attribute at all. However, I think your arguments are correct; please disregard this comment. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-468 >>> - if (!weight.isEmpty()) { >> >> I wish we could collapse all of these String::isEmpty() calls into a single call and re-use the result, but I'm not sure if it would actually be beneficial. > > Well, for isEmpty there’s a tiny chance that would compiler to tighter code. For isNull it likely wouldn’t because it’s just a simple null pointer check, and I’m not sure checking a boolean local variable would be better. Should decide based on code readability, not performance optimization. That's right; this comment was regarding code readability, not performance. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:506 >>> + write16(result, m_fontElement.fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()); >> >> So much better! However, when I actually do use the missing glyph info for this, I'll want to predicate this fallback based on whether or not parsing failed, which means I'll have to reintroduce some of this deleted code soon. > > If you do that, you should figure out exactly what you mean by “parsing failed”. We've already addressed this in https://bugs.webkit.org/show_bug.cgi?id=136971 >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:508 >>> + auto tableSizeOffset = result.size(); >> >> Conceptually this would be simpler if you moved this result.size() call after the line below that writes the placeholder. That way you wouldn't need the "- 2". > > We use tableSizeOffset twice below. If I moved this after the line below then I’d have to use tableOffsetSize - 2 in the first place i use it. I don’t think that’s conceptually simpler. We could have two offsets, but I’m not sure that would be better. You're right. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:509 >>> + write16(result, 0); // Place to write table size. >> >> nit: s/Place/Placeholder/ > > I meant “place”, not “placeholder”. I agree that the zero is a “placeholder”, but the place I am putting the zero is a “place”. So I chose this on purpose. Okay. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:512 >>> + if (int16_t verticalOriginY = glyph->fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()) { >> >> Probably should clamp this to a the range of an int16_t. >> >> Outputting the origin regardless of parsing success seems iffy to me. This make an assumption that the toInt() behavior returns the same value as an unknown vertical origin, which might not be correct based on the underlying platform implementation. > > Why is clamping to the range of an int16_t the right thing to do? Should a large value turn into 32767? Should a small value turn into -32768? What makes that useful behavior? I agree that simply truncating into an int16_t isn’t right, but I don’t know why clamping is right. > > The “iffy” thing you are referring to doesn’t exist. The code does not write out an origin when parsing fails. When parsing fails we get a zero and the if statement evaluates to false. > > On a separate note, I don’t know what an “unknown vertical origin” is. Regarding clamping: given a value that is out of the range of representable values in the output font, clamping will get us as close as possible to the value specified in the input font. Modulus is almost certainly worse. Regarding "iffy"ness: I'm interested in the case where the vert-origin-y value fails to parse. In the original code, no such entry would be written out in the font for it (since no pair would be appended to origins). In this new code, if parsing fails, you write out a 0 entry to the font, which could end up meaning something different to the platform's font parsing engine. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:518 >>> + overwrite16(result, tableSizeOffset, (result.size() - tableSizeOffset - 2) / 4); >> >> Shouldn't we assert that result.size() - tableSizeOffset % 4 == 0? (Assuming that you move the tableSizeOffset after the table size placeholder) > > Sure it would be OK to add an assertion, but I don’t think the assertion is all that valuable. I think it would be valuable as a consistency check for tableSizeOffset. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:-599 >>> - // FIXME: This can be made way faster >> >> We can still make this faster by not computing deltas in writePoint() if we are handed deltas in the first place. > > Maybe, but I’m not sure that “faster” is a meaningful issue here. I think the memory allocation in this overall operation is likely far more expensive than the floating point math. I don’t think this particular optimization opportunity is so valuable that we need to comment on it. > > Note also that if we are passed deltas, we still need to do math on two of them to convert them to deltas relative to each other, not deltas related to the start point. I don’t think there’s a compelling need to change this unless it’s producing incorrect results or really does measure as a significant performance cost. You are right. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:627 >>> + // FIXME: Shouldn't this be isNull? Is there a reason to handle an empty string attribute the same as a missing attribute? >> >> I gave my argument above about writing empty strings to the font. I suppose in this case, however, an empty string would result in the same result. > > I am not sure it’s really a correct argument. Why is empty string handled specially, but not, say, a string containing a single space in it? Instead of adding a comment, please go ahead and change it to isNull(). >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:628 >>> + // FIXME: Why is this different than what we do below when parsing fails. >> >> Good thought. It seems to me that the parsing fails code path is straight up broken right now. > > OK. I think we need unit tests for this code. Yep! Coming soon :) >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:710 >>> + if (segment == "bold") { >> >> I should probably make these a case insensitive compare > > Well, maybe. All depends on what the SVG specification says. SVG says the font-weight property has the "Same syntax and semantics as the ‘font-weight’ descriptor within an @font-face rule." http://www.w3.org/TR/SVG/fonts.html#FontFaceElementFontWeightAttribute CSS says "All CSS syntax is case-insensitive within the ASCII range" http://www.w3.org/TR/CSS2/syndata.html#characters >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:713 >>> + // Are we intentionally implementing a "last one wins" rule? >> >> Nope. The spec doesn't define what to do in this case. We might as well break here. > > We should come up with a good rule. I suppose “first wins” is better than “last wins”. "First wins" is probably better. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:718 >>> + // FIXME: Do we want to truncate, or would it be better to round? >> >> The spec only defines multiples of 100 as technically valid, so either way is probably fine. > > We need to define the behavior we want to implement for invalid values. We can’t write code that does three different things, so lets chose one. We could treat such values as invalid, or truncate them, or round them, or possibly do something else. Rounding is probably best. >>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:726 >>> + for (auto& segment : segments) { >> >> Seems like this could be joined up with the loop above it. I feel pretty sheepish for making two separate loops... > > Good point. I thought the loop was parsing two different strings. > > Note that joining the loops means not doing “break”, since we want to find both "bold" and "italic". After looking at the spec, this loop ::should:: be parsing two different strings. This one should be parsing the "font-style" attribute. >>> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:192 >>> + EXPECT_EQ(-2147483648, String("-2147483648").toInt(&ok)); >> >> Might want to test if the string appears to be larger than the largest int or smaller than the smallest int > > That is tested here already. 2147483648 (just before this test) is larger than the largest and -2147483649 (just after this test) is smaller than the smallest. > > Unless you mean something different? Oh, whoops. I had assumed that that is INT_MAX and INT_MIN.
Comment on attachment 238379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238379&action=review >>>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:512 >>>> + if (int16_t verticalOriginY = glyph->fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()) { >>> >>> Probably should clamp this to a the range of an int16_t. >>> >>> Outputting the origin regardless of parsing success seems iffy to me. This make an assumption that the toInt() behavior returns the same value as an unknown vertical origin, which might not be correct based on the underlying platform implementation. >> >> Why is clamping to the range of an int16_t the right thing to do? Should a large value turn into 32767? Should a small value turn into -32768? What makes that useful behavior? I agree that simply truncating into an int16_t isn’t right, but I don’t know why clamping is right. >> >> The “iffy” thing you are referring to doesn’t exist. The code does not write out an origin when parsing fails. When parsing fails we get a zero and the if statement evaluates to false. >> >> On a separate note, I don’t know what an “unknown vertical origin” is. > > Regarding clamping: given a value that is out of the range of representable values in the output font, clamping will get us as close as possible to the value specified in the input font. Modulus is almost certainly worse. > > Regarding "iffy"ness: I'm interested in the case where the vert-origin-y value fails to parse. In the original code, no such entry would be written out in the font for it (since no pair would be appended to origins). In this new code, if parsing fails, you write out a 0 entry to the font, which could end up meaning something different to the platform's font parsing engine. That’s incorrect. In the new code, if the parsing fails, verticalOriginY is zero and the code doesn’t write anything.
Comment on attachment 238379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238379&action=review >>>>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:512 >>>>> + if (int16_t verticalOriginY = glyph->fastGetAttribute(SVGNames::vert_origin_yAttr).toInt()) { >>>> >>>> Probably should clamp this to a the range of an int16_t. >>>> >>>> Outputting the origin regardless of parsing success seems iffy to me. This make an assumption that the toInt() behavior returns the same value as an unknown vertical origin, which might not be correct based on the underlying platform implementation. >>> >>> Why is clamping to the range of an int16_t the right thing to do? Should a large value turn into 32767? Should a small value turn into -32768? What makes that useful behavior? I agree that simply truncating into an int16_t isn’t right, but I don’t know why clamping is right. >>> >>> The “iffy” thing you are referring to doesn’t exist. The code does not write out an origin when parsing fails. When parsing fails we get a zero and the if statement evaluates to false. >>> >>> On a separate note, I don’t know what an “unknown vertical origin” is. >> >> Regarding clamping: given a value that is out of the range of representable values in the output font, clamping will get us as close as possible to the value specified in the input font. Modulus is almost certainly worse. >> >> Regarding "iffy"ness: I'm interested in the case where the vert-origin-y value fails to parse. In the original code, no such entry would be written out in the font for it (since no pair would be appended to origins). In this new code, if parsing fails, you write out a 0 entry to the font, which could end up meaning something different to the platform's font parsing engine. > > That’s incorrect. In the new code, if the parsing fails, verticalOriginY is zero and the code doesn’t write anything. You're right; however, the reverse problem is now true: vertical origins that are specified as 0 don't get written out.
Created attachment 238753 [details] Patch
(In reply to comment #8) > You're right; however, the reverse problem is now true: vertical origins that are specified as 0 don't get written out. That's not a change in behavior. The code before my patch is already checking for zero and not writing out vertical origins based on that. It says: if (ok && verticalOriginY) If that is wrong, I’d be happy to fix it. But I assumed it was intentional.
(In reply to comment #10) > (In reply to comment #8) > > You're right; however, the reverse problem is now true: vertical origins that are specified as 0 don't get written out. > > That's not a change in behavior. The code before my patch is already checking for zero and not writing out vertical origins based on that. It says: > > if (ok && verticalOriginY) > > If that is wrong, I’d be happy to fix it. But I assumed it was intentional. You're right! Though this behavior is wrong: any vertical origin that is specified (in a way that we can parse as a number) should be specified in the output. Because the problem isn't actually in this patch, I can fix this in a subsequent patch if you like.
Comment on attachment 238753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238753&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:84 > + , charString(WTF::move(charString)) Will this actually work? It looks like charString is getting copied in to the function anyway - this is only saving the copy from the function to the member variable. Or are constructor initializer lists special? Making the charString an rvalue reference might help with readability in either case. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:334 > + int value = m_fontElement.fastGetAttribute(SVGNames::horiz_adv_xAttr).toInt(&ok); Instead of getting the attribute, you can use m_fontFaceElement->horizontalAdvanceX() > Source/WebCore/svg/SVGToOTFFontConversion.cpp:871 > + m_glyphs.append(GlyphData(WTF::move(path), glyphElement, horizontalAdvance, verticalAdvance, m_boundingBox, codepoint)); I think it would be beneficial to only have one argument and conditionally use downcast<>(), rather than having two arguments that mean almost the same thing but are subtly different. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:913 > + // Indicate error by leaving the glyphs vector empty. A comment at all might not be necessary; it's pretty obvious what's going on. Up to you.
r=me
Comment on attachment 238753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238753&action=review Myles said r=me but didn’t change the review state to +. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:84 >> + , charString(WTF::move(charString)) > > Will this actually work? It looks like charString is getting copied in to the function anyway - this is only saving the copy from the function to the member variable. Or are constructor initializer lists special? Making the charString an rvalue reference might help with readability in either case. Yes, it will work. The caller moves into the argument, the function moves into the data member. You can’t see the move at the call site here, but it’s there. Changing this to an rvalue reference would also be fine with me. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:334 >> + int value = m_fontElement.fastGetAttribute(SVGNames::horiz_adv_xAttr).toInt(&ok); > > Instead of getting the attribute, you can use m_fontFaceElement->horizontalAdvanceX() That would be a big change. For one, it returns a float, not an integer, for another, it returns 0 if there is no font element, and doesn’t provide a separate “is OK” flag. If you want to make that change, you’ll need to do it. It’s not just refactoring. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:871 >> + m_glyphs.append(GlyphData(WTF::move(path), glyphElement, horizontalAdvance, verticalAdvance, m_boundingBox, codepoint)); > > I think it would be beneficial to only have one argument and conditionally use downcast<>(), rather than having two arguments that mean almost the same thing but are subtly different. Which two arguments are you talking about? Is this comment about the processGlyphElement function below? I actually think the current design of that function is great, but sure you could check at runtime instead of compile time if you prefer. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:913 >> + // Indicate error by leaving the glyphs vector empty. > > A comment at all might not be necessary; it's pretty obvious what's going on. Up to you. OK, I’ll omit that comment.
> Myles said r=me but didn’t change the review state to +. Myles is not an official reviewer yet. I considered giving a rubber-stamp, but given how little I understand about this patch, that seemed silly. Perhaps Mitz could weigh in.
Committed r174063: <http://trac.webkit.org/changeset/174063>
Oops, I landed it without a review then. Given this is new code and not yet turned on, I think this is OK.