Bug 11474 - Rename the "p" member variable of the PaintInfo struct to "context"
Summary: Rename the "p" member variable of the PaintInfo struct to "context"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-31 15:23 PST by Sam Weinig
Modified: 2006-11-01 14:08 PST (History)
1 user (show)

See Also:


Attachments
patch (193.47 KB, patch)
2006-10-31 16:19 PST, Sam Weinig
mitz: review-
Details | Formatted Diff | Diff
updated patch (202.22 KB, patch)
2006-11-01 08:09 PST, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2006-10-31 15:23:43 PST
Patch forthcoming.
Comment 1 Sam Weinig 2006-10-31 16:19:52 PST
Created attachment 11314 [details]
patch

Patch renames the 'p' member to 'context' and the 'r' member to 'rect'.  No changelog has been to added yet as while I was making the patch several changes were made to near by rendering code that I will have to account for.  I'll get an updated patch up asap.
Comment 2 mitz 2006-11-01 04:19:48 PST
Comment on attachment 11314 [details]
patch

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; };
Comment 3 Sam Weinig 2006-11-01 08:09:44 PST
Created attachment 11327 [details]
updated patch

The updated patch addresses Mitz's concerns, is updated for the current ToT, and adds a ChangeLog.
Comment 4 mitz 2006-11-01 08:27:16 PST
Comment on attachment 11327 [details]
updated patch

r=me
Comment 5 Sam Weinig 2006-11-01 14:08:14 PST
Landed in r17521.