[Webkit-unassigned] [Bug 11506] Cleanup RenderObject
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Nov 3 12:44:53 PST 2006
http://bugs.webkit.org/show_bug.cgi?id=11506
mitz at webkit.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #11366|review? |review-
Flag| |
------- Comment #3 from mitz at webkit.org 2006-11-03 12:44 PDT -------
(From update of attachment 11366)
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;
--
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.
More information about the webkit-unassigned
mailing list