5 ms·

From the fix: https://bugs.webkit.org/attachment.cgi?id=448463&action=prettypatch https://bugs.webkit.org/attachment.cgi?id=448463&action=pret... > Reviewed by

From the fix: https://bugs.webkit.org/attachment.cgi?id=448463&action=prettypatch https://bugs.webkit.org/attachment.cgi?id=448463&action=pret... > Reviewed by NOBODY (OOPS!). If I was reviewing the code, I would have asked why Myles changed WidthIterator.cpp's call to charactersTreatedAsSpace.constructAndAppend from a separate parameter on each line to everything inline. This change made the diff of this change harder to read, and will make the diff of any future parameter change harder to read also. I also would have created an intermediary variable or a helper method for `character == tabCharacter ? width : font.spaceWidth()` - it's not clear at all here why this check needs to be there (without looking at the ChangeLog, which I shouldn't have to do to understand code), or why the old behaviour that used `width` instead of `font.spaceWidth()` ONLY applies to tabCharacter (So it should be encapsulate inside a method with a clear comment with the info from the ChangeLog) Lastly, this diff doesn't seem to match the blog description: > The font also has data about the newline character, including its width. This doesn’t really make sense – new lines don’t (or at least shouldn’t) take up any space, but the font doesn’t treat it differently to any other character. The creator of the font still has to include a width for the new line character in the font’s data. And in the font we use on GOV.UK, the new line character has a bigger width than a space character – which is apparently unusual. This makes it sound like the FONT is wrong and the font width shouldn't be used for width calculation. Yet the change in the code introduces using the font's space width explicitly. Ultimately the ChangeLog doesn't match the code. I trust the blog writer, the code author, and the description of the bug in the ChangeLog. What needed improvement here then is the code to imperatively match what is described in English.

madeofpalk 4y ago
> it's not clear at all here why this check needs to be there (without looking at the ChangeLog, which I shouldn't have to do to understand code) Teams that standardise on in-depth change messages usually have a culture to look towards commit messages/changelogs to understand more motivation about why code is a particularly way. > This makes it sound like the FONT is wrong and the font width shouldn't be used for width calculation. Yet the change in the code introduces using the font's space width explicitly. I don't know C++, and I'm not familiar with the codebase, but my read of the code change says that "if it's a 'treat-it-like-a-space-character" that isn't a tab, then size the character like a space character". This is inline with the english description
- saagarjha 4y ago
  That is an accurate description of this code.
saagarjha 4y ago
> The font also has data about the newline character, including its width. This doesn’t really make sense [emphasis mine] The font doesn’t have one width, it has a width for each character. The data for the newline character is bogus. All of these match.

madeofpalk 4y ago

> it's not clear at all here why this check needs to be there (without looking at the ChangeLog, which I shouldn't have to do to understand code) Teams that standardise on in-depth change messages usually have a culture to look towards commit messages/changelogs to understand more motivation about why code is a particularly way. > This makes it sound like the FONT is wrong and the font width shouldn't be used for width calculation. Yet the change in the code introduces using the font's space width explicitly. I don't know C++, and I'm not familiar with the codebase, but my read of the code change says that "if it's a 'treat-it-like-a-space-character" that isn't a tab, then size the character like a space character". This is inline with the english description

saagarjha 4y ago
That is an accurate description of this code.

saagarjha 4y ago

> The font also has data about the newline character, including its width. This doesn’t really make sense [emphasis mine] The font doesn’t have one width, it has a width for each character. The data for the newline character is bogus. All of these match.