[webkit-reviews] review denied: [Bug 3692] Word-spacing doesn't work as expected : [Attachment 3207] More reformatting

[bugzilla-request-daemon at opendarwin.org]

Dave Hyatt <hyatt at apple.com> has denied Oliver Hunt
<ojh16 at student.canterbury.ac.nz>'s request for review:
Bug 3692: Word-spacing doesn't work as expected
http://bugzilla.opendarwin.org/show_bug.cgi?id=3692

Attachment 3207: More reformatting
http://bugzilla.opendarwin.org/attachment.cgi?id=3207&action=edit

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
Patch seems ok conceptually.  Just needs some clean-up in terms of formatting. 
Some comments:

(1) There's a lot of 2-space indentation being used.  Our standard is 4-space
indentation, so go over the patch and make sure the indentation is correct.

(2) You patched some dead code (slated for removal) in font.cpp.  I don't see
any reason to update that code path.  You can just leave those changes out of
the patch.

(3) Make sure to match our coding conventions.	I know a lot of the surrounding
code does not, but make sure new code does.  For example, this line:

+	     if ( (r->start == 0) && needsWordSpacing &&
(rt->text()[r->start].isSpace()) ) {
+	       effectiveWidth += rt->htmlFont(m_firstLine)->getWordSpacing();
+	     }

Get rid of the extra spaces after the opening of the if ( and before the ). 
Also get rid of the unnecessary braces.

(5) For this line:

+    if (needsWordSpacing && len>1) 


len > 1 rather than len>1.  Yes, I know a lot of the surrounding code doesn't
obey current conventions, but it's all due for a big cleanup soon. :)

Logically everything looks great.  Let's just get all the formatting nitpicks
out of the way and then we can get this landed.

I do think (since compacts have been touched a bit) that the word-spacing uber
test should test display: compact to make sure word-spacing is properly
factoring in to the calculations for whether or not they can fit in the block's
margin.