[webkit-reviews] review denied: [Bug 13234] Layout of selected justified text is broken : [Attachment 13897] Patch that eliminates run.from() and run.to() in favor of passing them in as args only when needed

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Dave Hyatt <hyatt at apple.com>'s request for review:
Bug 13234: Layout of selected justified text is broken
http://bugs.webkit.org/show_bug.cgi?id=13234

Attachment 13897: Patch that eliminates run.from() and run.to() in favor of
passing them in as args only when needed
http://bugs.webkit.org/attachment.cgi?id=13897&action=edit

------- Additional Comments from mitz at webkit.org
+    // Using roundf() rather than ceilf() for the right edge as a compromise
to ensure correct caret positioning
+    if (style.rtl()) {
+	 it.advance(run.length());
+	 startX += it.m_finalRoundingWidth - afterWidth;

The copy&pasted comment doesn't make sense there. Also, I think you need to
cache the finalRoundingWidth before advancing to the end and use that (like
floatWidthForSimpleText() did), otherwise RTL text will jiggle as you select it
(if it's painted separately). Finally, you forgot to add it.m_runWidthSoFar
there (that's why you advance to the end). So I think the correct code would be


+    if (style.rtl()) {
+	 float finalRoundingWidth = it.m_finalRoundingWidth;
+	 it.advance(run.length());
+	 startX += finalRoundingWidth + it.m_runWidthSoFar - afterWidth;


+	     if (Font::isRoundingHackCharacter(nextCh) && (!isLastChar ||
params->m_style.applyRunRounding())){

Missing space before the { there.

Please include a test for the justified text bug (similar to the one for bug
13224).