[webkit-reviews] review denied: [Bug 11844] Code Cleanup for more of the rendering code : [Attachment 11869] patch

[bugzilla-request-daemon at macosforge.org]

mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 11844: Code Cleanup for more of the rendering code
http://bugs.webkit.org/show_bug.cgi?id=11844

Attachment 11869: patch
http://bugs.webkit.org/attachment.cgi?id=11869&action=edit

------- Additional Comments from mitz at webkit.org
Move the ampersand next to 'int' here:

 InlineTextBox* RenderText::findNextInlineTextBox(int offset, int &pos) const

Make this "the RenderText's m_str" (without 'string'). Also I don't think the
'@p' further down helps.

-    // The text runs point to parts of the rendertext's str string
+    // The text runs point to parts of the rendertext's m_str string
     // (they don't include '\n')
     // Find the text run that includes the character at @p offset
     // and return pos, which is the position of the char in the run.

Move this '*':

 IntRect RenderText::caretRect(int offset, EAffinity affinity, int
*extraWidthToEndOfLine)

What does this comment tell you that the code that follows it doesn't? If
you're leaving it in, then since you touched it I'll have to ask you to
capitalize it.

+    // if the text has a variable width tab, we need to call
     if (m_hasTab)
	 calcMinMaxWidth(leadWidth);

Please break this into two statements:

	 beginMaxW = endMaxW = maxW;

Need spaces around the first '+' too:

+	     while (i+linelen < len && (*m_str)[i + linelen] != '\n')

Split:

     m_hasBreakableChar = m_hasBreak = m_hasTab = m_hasBeginWS = m_hasEndWS =
false;

Can the local variable be renamed 'wordLen'?

	 int wordlen = j - i;

Needs spaces around '+':

+	  currPos < from+len && ((*m_str)[currPos] == '\n' || (*m_str)[currPos]
== ' ' || (*m_str)[currPos] == '\t');

How about using INT_MAX, then? Also, 'pos', 'x', 'result' and 'retVal' are all
nicer names for the local variable.

+    // FIXME: we should not use a random value like this.
+    int retval = 6666666;

I think it's better to indent the whole block after 'case CAPITALIZE:' and
leave the 'break;' outside:

	     switch (style()->textTransform()) {
		 case CAPITALIZE:

Consider using INT_MAX:

+    // FIXME: we should not use a random value like this.
     int minx = 100000000;

This should say 'in case':

+    // If we're within the text control, we want to act as if we've hit the
inner div, incase the point

It would be nice to change this to say RenderTextControl and update test
results at some point.
 
+    virtual const char* renderName() const { return "RenderTextField"; }

These changes are fine (unless you think it should be '255.0'?). However, I
wonder if the math is correct - it maps 0.9999 to 254, which seems wrong (that
is, I don't think that's how CG works):

-    return Color((int)(255 * [color redComponent]), (int)(255 * [color
greenComponent]), (int)(255 * [color blueComponent]));
+    return Color(static_cast<int>(255 * [color redComponent]),
static_cast<int>(255 * [color greenComponent]), static_cast<int>(255 * [color
blueComponent]));

How about 'ts << "layer " << layerBounds;' instead of this?

+    ts << "layer" << " " << layerBounds;

This should be 'true':

+    , m_printImages(false)

Please rename 'f' to 'fixed' here too:

+    virtual bool absolutePosition(int& xPos, int& yPos, bool f = false);

Needs spaces around the first '||':

+	 if (topLevel||!(text->x()->getFirst() || text->y()->getFirst() ||

There is no KRenderingStyle.cpp:

+// FIXME: This should be in KRenderingStyle.cpp

+// FIXME: This should be in KRenderingStyle.cpp

+// FIXME: This should be in KRenderingStyle.cpp