Bug 11844 – Code Cleanup for more of the rendering code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sam Weinig]

Patch forthcoming

[Sam Weinig]

Created attachment 11869 [details]
patch

Cleanup patch.

Comment on attachment 11869 [details]
patch

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

[Sam Weinig]

Created attachment 11887 [details]
updated patch

Updated patch addressing mitz's comments.

Comment on attachment 11887 [details]
updated patch

r=me, although I'd prefer if you addressed the following comments before landing:

Use the class name 'RenderText' instead of 'rendertext':

+    // The text runs point to parts of the rendertext's m_str

I'm fine with you just adding these comments for now, but it's wrong to call these values 'random'. Use 'arbitrary' instead.

+    // FIXME: we should not use a random value like this.  Perhaps we should use INT_MAX.

+    // FIXME: we should not use a random value like this.  Perhaps we should use INT_MAX.

Here I think it's best to leave 'sizes' since otherwise it's not obvious that the parameter is pointing to an array:

-    void setControlSize(NSCell*, const IntSize* sizes, const IntSize& minSize);
-    void setSizeFromFont(RenderStyle*, const IntSize* sizes) const;
-    IntSize sizeForFont(RenderStyle*, const IntSize* sizes) const;
-    IntSize sizeForSystemFont(RenderStyle*, const IntSize* sizes) const;
+    void setControlSize(NSCell*, const IntSize*, const IntSize& minSize);
+    void setSizeFromFont(RenderStyle*, const IntSize*) const;
+    IntSize sizeForFont(RenderStyle*, const IntSize*) const;
+    IntSize sizeForSystemFont(RenderStyle*, const IntSize*) const;

[Sam Weinig]

Landed in r18263.