[webkit-reviews] review denied: [Bug 11474] Rename the "p" member variable of the PaintInfo struct to "context" : [Attachment 11314] patch

[bugzilla-request-daemon at macosforge.org]

mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 11474: Rename the "p" member variable of the PaintInfo struct to "context"
http://bugs.webkit.org/show_bug.cgi?id=11474

Attachment 11314: patch
http://bugs.webkit.org/attachment.cgi?id=11314&action=edit

------- Additional Comments from mitz at webkit.org
Wow! Very few comments:

This can be replaced with 'RenderStyle* style = m_object->style(m_firstLine)':

+    RenderStyle* style = m_firstLine ? m_object->firstLineStyle() :
m_object->style();

This is not equivalent and probably wrong:

-    if (inlineFlow)
-	 ASSERT(m_layer); // The only way a compact/run-in/inline could paint
like this is if it has a layer.
+    ASSERT(inlineFlow && m_layer); // The only way a compact/run-in/inline
could paint like this is if it has a layer.

(the above change appears twice in the patch)

You didn't change this, but I don't understand why you need to round trip
through RGBA:

+	 context->setFillColor(color.rgb());

+	 context->setFillColor(color.rgb());

This multi-line for loop could use braces:

	     for (RenderObject* child = firstChild(); child; child =
child->nextSibling())
		 if (child->isTableSection())
-		     child->paint(paintInfo, _tx, _ty);
+		     child->paint(info, tx, ty);

Missing space before the =:

+	 mh= max(0, h - (paintInfo.rect.y() - ty));

For consistency (within this patch, at least) the || should go on the end of
the first line:

-    if (i.phase == PaintPhaseCollapsedTableBorders && style()->visibility() ==
VISIBLE) {
-	 if (_ty - table()->outerBorderTop() >= i.r.bottom() + os || _ty +
_topExtra + m_height + _bottomExtra + table()->outerBorderBottom() <= i.r.y() -
os)
+    if (paintInfo.phase == PaintPhaseCollapsedTableBorders &&
style()->visibility() == VISIBLE) {
+	 if (ty - table()->outerBorderTop() >= paintInfo.rect.bottom() + os
+		 || ty + _topExtra + m_height + _bottomExtra +
table()->outerBorderBottom() <= paintInfo.rect.y() - os)

I think the Pen() is redundant here:

+    paintInfo.context->setPen(Pen(o->style()->color()));

+    paintInfo.context->setPen(Pen(leftSeparatorColor));

+    paintInfo.context->setPen(Pen(rightSeparatorColor));

No need for the last semicolon:

+    virtual bool isWidget() const { return true; };