Bug 11441 – More rendering code cleaning

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

[Sam Weinig]

More fall cleaning of the rendering code.  Patch to come.

[Sam Weinig]

Created attachment 11262 [details]
patch

[Sam Weinig]

Created attachment 11263 [details]
no-whitespace-changes version of the patch

Diff made with svn diff -x -w

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)

Finally, I don't think the patch will merge following r17399 :(

[Sam Weinig]

Created attachment 11279 [details]
updated patch

Comment on attachment 11279 [details]
updated patch

Great work! I have 3 comments which you should fix when landing.

"its", not "it's":

+// FIXME: move this to it's own file

"IntRect &exposeRect" -> "IntRect& exposeRect":

+    IntRect getRectToExpose(const IntRect& visibleRect,  const IntRect &exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY);    

'b' is a bad name, but 'firstLine' is better than no name:

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

[Sam Weinig]

Landed in r17448.