[Webkit-unassigned] [Bug 11441] More rendering code cleaning

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=11441


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mitz at webkit.org




------- Comment #3 from mitz at webkit.org  2006-10-28 11:35 PDT -------
I actually had a bunch of comments on the first 2/3 of the patch before Tim
reviewed it :-)

In case you want to consider them:

I interpret the "one line" guideline as "one line", not "one statement", so
revert this:

-    
-    if (!result) {
+
+    if (!result)
         // Allocate a new chunk from the arena
         ARENA_ALLOCATE(result, &m_pool, size);
-    }

Please change 'b' to 'firstLine' in these:

+    virtual short lineHeight(bool b, bool isRootLineBox = false) const;
+    virtual short baselinePosition(bool b, bool isRootLineBox = false) const;

For this:

-    virtual int maxTopMargin(bool positive) const {
+    virtual int maxTopMargin(bool positive) const
+    {
         if (positive)
             return m_maxTopPosMargin;
-        else
-            return m_maxTopNegMargin;
+        return m_maxTopNegMargin;
     }

I think "return positive ? m_maxTopPosMargin : m_maxTopNegMargin;" is neater.
Ditto for maxBottomMargin().

Leftover space after the &:

+    int skipWhitespace(BidiIterator& , BidiState&);

'stream' is redundant:

+    virtual void dump(TextStream* stream, DeprecatedString ind = "") const;

I don't think you need parameter names at all in these:
+    RootInlineBox* lineAtIndex(int index);
+    int heightForLineCount(int lineCount);

Same goes for 'renderer' here:
+    Position positionForRenderer(RenderObject* renderer, bool start = true)
const;

Is this comment still needed?!

-    // XXX Generalize to work with top and left as well.
+    // FIXME: Generalize to work with top and left as well.

And what about this one? Duh.

+#include "AXObjectCache.h" // For accessibility

Index: WebCore/rendering/RenderFieldset.cpp

Needs spaces around the minus sign:

+    int yOff = (legend->yPos() > 0) ? 0 : (legend->height()-borderTop()) / 2;

Index: WebCore/rendering/RenderForeignObject.cpp

I wanted to suggest a better name for the bool in:

+    virtual void computeAbsoluteRepaintRect(IntRect&, bool f);

Apparently it means that the caller is fixed, but I can't see that it's being
used anywhere. Weird.

The change makes the comment redundant:

-    ASSERT(false); // Should never be reached.
+    ASSERT_NOT_REACHED(); // Should never be reached.

I don't like the name 'needsLayout' here:

-    bool needlayout = false;
+    bool needsLayout = false;

How about 'ensureLayout'?

Double parentheses:

+    if ((newImage->imageSize().width() != intrinsicWidth() ||
newImage->imageSize().height() != intrinsicHeight() || imageSizeChanged)) {

Missing space after if:

+            if(imageSizeChanged || m_width != oldwidth || m_height !=
oldheight)


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.