[webkit-reviews] review granted: [Bug 235559] REGRESSION(r281419): iCloud.com Notes web app fonts render incorrectly : [Attachment 449899] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 235559: REGRESSION(r281419): iCloud.com Notes web app fonts render
incorrectly
https://bugs.webkit.org/show_bug.cgi?id=235559

Attachment 449899: Patch

https://bugs.webkit.org/attachment.cgi?id=449899&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 449899
  --> https://bugs.webkit.org/attachment.cgi?id=449899
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449899&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:608
> +	   // "Control characters (Unicode category Cc)âother than NULL
(U+0000), tabs (U+0009), line feeds (U+000A), carriage returns (U+000D) and
sequences that form a segment breakâmust be rendered as a visible glyph"

Seems like this "counts your chickens before they are hatched" since it assumes
your pull request will be approved. If I was you I would be tempted to write a
brief comment that more literally explains the current status of this rather
than assuming the change to the spec will go through.

> Source/WebCore/platform/graphics/WidthIterator.cpp:610
> +	       && u_charType(characterResponsibleForThisGlyph) ==
U_CONTROL_CHAR) {

Does this really reject tabs, line feeds, carriage returns, and sequences that
form a segment break? The comment above lists all those special cases, and the
code here doesn’t cover them. It seems like the comment might need to explain
why the code below doesn’t match it?