[webkit-reviews] review denied: [Bug 20092] Spelling markers positioned incorrectly in RTL text : [Attachment 23682] Patch for review

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 20092: Spelling markers positioned incorrectly in RTL text
https://bugs.webkit.org/show_bug.cgi?id=20092

Attachment 23682: Patch for review
https://bugs.webkit.org/attachment.cgi?id=23682&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
A patch needs to include a test case and a ChangeLog entry. This doesn't have
either. Dan Bernstein can help you make a "pixel test" for this, or you could
construct a manual test case for the manual-tests directory.

 659	 int startPosition = max(marker.startOffset - m_start, (unsigned)0);

This is wrong. Since both expressions are of type "unsigned", then it's going
to be an unsigned "max", so it will always be startOffset - start, even if it's
a negative number. Instead of casting the right side to unsigned we need to
cast the left side to int.

I'd like to see test cases that exercise this "max" statement. Or we can leave
the max out if it's not really needed -- if we can't create a test case that is
affected then maybe it's not.

 660	 int endPosition = min(marker.endOffset - m_start, (unsigned)m_len);

 677	     int grammarEndPosition = min(marker.endOffset - m_start,
(unsigned)m_len);

Might be the same problem here. We don't want a negative number to turn into
m_len, so we need to do an int version of min instead of unsigned.

It occurs to me that another way to accomplish this is to call min<int>(a, b)
instead of casting a or b to an unsigned.

 662	 if (m_truncation != cNoTruncation) {
 663	     endPosition = min(endPosition, startPosition + m_truncation);
 664	 }

Out style guide doesn't use braces around the bodies of single line if
statements.

review- because of the issues above