Bug 11506 – Cleanup RenderObject

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

[Sam Weinig]

RenderObject is a big enough class that I thought it would be easier to put it's cleanup by itself.  Patch forthcoming.

[Sam Weinig]

Created attachment 11365 [details]
patch

[Sam Weinig]

Created attachment 11366 [details]
updated patch

Comment on attachment 11366 [details]
updated patch

If you split this into two lines, it needs to have braces. I'm not sure you need to split it, though.

     if (bgLayer->next())
-        return true; // Nobody will use multiple background layers without wanting fancy positioning.
-    
+        // Nobody will use multiple background layers without wanting fancy positioning.
+        return true;
+

Ditto:

-    if (shouldPaintBackgroundImage && 
-        (bgLayer->backgroundXPosition().value() != 0 || bgLayer->backgroundYPosition().value() != 0
-        || bgLayer->backgroundSize().width.isPercent() || bgLayer->backgroundSize().height.isPercent()))
-        return true; // The background image will shift unpredictably if the size changes.
-        
+    if (shouldPaintBackgroundImage &&
+            (bgLayer->backgroundXPosition().value() != 0 || bgLayer->backgroundYPosition().value() != 0
+             || bgLayer->backgroundSize().width.isPercent() || bgLayer->backgroundSize().height.isPercent()))
+        // The background image will shift unpredictably if the size changes.
+        return true;
+

And another one:

         if (shouldPaintBorderImage && borderImage->isLoaded())
-            return true; // If the image hasn't loaded, we're still using the normal border style.
+            // If the image hasn't loaded, we're still using the normal border style.
+            return true;

Another option for these is to put each comment before the if statement. Maybe change the middle one to say "If the size changes, the background image will shift unpredictably".

Too many parentheses. '((thickness / 2) < 3)' can be rewritten as 'thickness / 2 < 2'. Similarly on the next line.

+    if ((style == DOUBLE && ((thickness / 2) < 3)) ||
+            ((style == RIDGE || style == GROOVE) && ((thickness / 2) < 2)))
         style = SOLID;

This code is doing the same thing regardless of s:

                 if (s == BSBottom || s == BSTop)
-                    p->drawArc(IntRect(x, y, radius.width() * 2, radius.height() * 2), thickness, angleStart, angleSpan);
+                    graphicsContext->drawArc(IntRect(x, y, radius.width() * 2, radius.height() * 2), thickness, angleStart, angleSpan);
                 else //We are drawing a left or right border
-                    p->drawArc(IntRect(x, y, radius.width() * 2, radius.height() * 2), thickness, angleStart, angleSpan);
+                    graphicsContext->drawArc(IntRect(x, y, radius.width() * 2, radius.height() * 2), thickness, angleStart, angleSpan);

These rgb() are unneeded:

+            graphicsContext->setFillColor(c.rgb());

+            graphicsContext->setFillColor(c2.rgb());

+            graphicsContext->setFillColor(c.rgb());

I really prefer '// fall through' to '/* nobreak; */' but there's no guideline about it.

+        case DOTTED:
+            graphicsContext->setPen(Pen(c, width == 1 ? 0 : width, Pen::DotLine));
+            /* nobreak; */
+        case DASHED:
+            if(style == DASHED)
+                graphicsContext->setPen(Pen(c, width == 1 ? 0 : width, Pen::DashLine));

But perhaps you can write it differently as

+        case DOTTED:
+        case DASHED:
+            graphicsContext->setPen(Pen(c, width == 1 ? 0 : width, style == DASHED ? Pen::DashLine : Pen::DotLine));

Isn't the brace supposed to go on the same line?

+        case DOUBLE:
+        {
+            int third = (width + 1) / 3;

Again, no rgb() needed:

+                graphicsContext->setFillColor(c.rgb());

More of these:

+            /* nobreak; */

+            /* nobreak; */

Missing space before the second '?':

+                   ignore_left ? 0 : style->borderLeftWidth(), ignore_right? 0 : style->borderRightWidth());

Too many parentheses. I don't see why 'bs >= OUTSET' needs to be on a separate line:

+            ((bc == lc) && (bt == lt) &&
+            (bs >= OUTSET) &&
+            (ls == DOTTED || ls == DASHED || ls == SOLID || ls == OUTSET));

Missing spaces after the ':'s:

+                   ignore_left ? 0 :style->borderLeftWidth(), ignore_right? 0 :style->borderRightWidth());

Again, if this comment is even necessary, I don't see why it should be split off:

     if (renderRadii)
-        p->restore(); // Undo the clip.
+        // Undo the clip.
+        graphicsContext->restore();

Please keep the braces around multi-line blocks:

-            } else if (curr->shouldSelect()) {
+            } else if (curr->shouldSelect())
                 // In this case we have a click in the unselected portion of text.  If this text is
                 // selectable, we want to start the selection process instead of looking for a parent
                 // to try to drag.
                 return 0;
-            }

-                if (style->position() == StaticPosition) {
+                if (style->position() == StaticPosition)
                     // Clear our positioned objects list. Our absolutely positioned descendants will be
                     // inserted into our containing block's positioned objects list during layout.
                     removePositionedObjects(0);
-                } else if (m_style->position() == StaticPosition) {
+                else if (m_style->position() == StaticPosition) {

Unneeded space after unary minus:

+            vpos += - static_cast<int>(f.xHeight() / 2) - lineHeight(firstLine) / 2 + baselinePosition(firstLine);

More of these comment splits...

         if (view() && element() && (element()->hasTagName(htmlTag) || element()->hasTagName(bodyTag)))
-            view()->repaint();    // repaint the entire canvas since the background gets propagated up
+            // repaint the entire canvas since the background gets propagated up
+            view()->repaint();
         else
-            repaint();              // repaint object, which is a box or a container with boxes inside it
+            // repaint object, which is a box or a container with boxes inside it
+            repaint();
     }
 }

Any reason why you didn't move this to the top like you did in the other classes?

+    virtual const char* renderName() const { return "RenderObject"; }

Please add spaces around the minus signs:

+    virtual int collapsedMarginTop() const { return maxTopMargin(true)-maxTopMargin(false); }
+    virtual int collapsedMarginBottom() const { return maxBottomMargin(true)-maxBottomMargin(false); }

Consider using max() and min() for these, and no '0 -'.

+            return (marginTop() > 0) ? marginTop() : 0;

+            return (marginTop() < 0) ? 0 - marginTop() : 0;

+            return (marginBottom() > 0) ? marginBottom() : 0;

+            return (marginBottom() < 0) ? 0 - marginBottom() : 0;

[Sam Weinig]

Created attachment 11432 [details]
patch v3

Update to address Mitz's comments.

Comment on attachment 11432 [details]
patch v3

r=me assuming you ran the layout tests

[Sam Weinig]

Landed in r17677.