Bug 60739 – Switch paintCollapsedBorder to use IntRect

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

It currently takes four ints representing a rect. This is the *last* function with this bad signature! :)

[Levi Weintraub]

Created attachment 93372 [details]
Patch

[Levi Weintraub]

Comment on attachment 93372 [details]
Patch

Oops, made a mistake :p

[Early Warning System Bot]

Comment on attachment 93372 [details]
Patch

Attachment 93372 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8688895

[WebKit Review Bot]

Comment on attachment 93372 [details]
Patch

Attachment 93372 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8687849

[WebKit Review Bot]

Comment on attachment 93372 [details]
Patch

Attachment 93372 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8688903

[WebKit Review Bot]

Comment on attachment 93372 [details]
Patch

Attachment 93372 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8687872

[Collabora GTK+ EWS bot]

Comment on attachment 93372 [details]
Patch

Attachment 93372 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8685931

[Levi Weintraub]

Created attachment 93386 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 93386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93386&action=review

I don't think the adjustedPaintRect change is a ncessarily a win.

> Source/WebCore/rendering/RenderTableCell.cpp:931
> +    IntRect adjustedPaintRect = IntRect(paintRect.x() - leftWidth / 2,

You don't have to do an equal here, you can just construct directly.  Webkit doesn't normally do cascading indents like this though.

> Source/WebCore/rendering/RenderTableCell.cpp:952
> +    borders.addBorder(topVal, BSTop, renderTop, adjustedPaintRect.x(), adjustedPaintRect.y(), adjustedPaintRect.maxX(), adjustedPaintRect.y() + topWidth, topStyle);
> +    borders.addBorder(bottomVal, BSBottom, renderBottom, adjustedPaintRect.x(), adjustedPaintRect.maxY() - bottomWidth, adjustedPaintRect.maxX(), adjustedPaintRect.maxY(), bottomStyle);
> +    borders.addBorder(leftVal, BSLeft, renderLeft, adjustedPaintRect.x(), adjustedPaintRect.y(), adjustedPaintRect.x() + leftWidth, adjustedPaintRect.maxY(), leftStyle);
> +    borders.addBorder(rightVal, BSRight, renderRight, adjustedPaintRect.maxX() - rightWidth, adjustedPaintRect.y(), adjustedPaintRect.maxX(), adjustedPaintRect.maxY(), rightStyle);

Seems like making the locals above might make this more clear?  Unleass you plan to actually use the adjustedPaintRect as an object somewhere?

[Levi Weintraub]

Created attachment 93484 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 93484 [details]
Patch

LGTM.

[Levi Weintraub]

Comment on attachment 93484 [details]
Patch

Clearing flags on attachment: 93484

Committed r86449: <http://trac.webkit.org/changeset/86449>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

(In reply to comment #11)
> (From update of attachment 93484 [details])
> LGTM.

Thanks for the review!