[webkit-reviews] review denied: [Bug 13632] Overflow scrolling doesn't account for bottom padding : [Attachment 28546] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 12 15:54:02 PDT 2009


Eric Seidel <eric at webkit.org> has denied Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 13632: Overflow scrolling doesn't account for bottom padding
https://bugs.webkit.org/show_bug.cgi?id=13632

Attachment 28546: Patch
https://bugs.webkit.org/attachment.cgi?id=28546&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I'd like to see "bottom" and "right" renamed to lowestPosition and
rightmostPosition to match the function names.	But I'm not holding my breath
(or holding you from committing), given that you're trying to make this patch
as small as possible.

All of your lp and rp variables I would have given clearer names:

int lp = lastLineBox()->y() + lastLineBox()->height();
int bottomOfLastLineBox = ...


I'm surprised this isn't already a method on RenderBox:
+	     RenderBox* currBox = lastChildBox();
+	     while (currBox && currBox->isFloatingOrPositioned())
+		 currBox = currBox->previousSiblingBox();

RenderBox* lastNormalFlowChildBox();

the collapsedMarginBottom code is wrong (as discussed on IRC).

The paddingBottom() addition here
bottom = max(bottom, lp + paddingBottom());
, shoudl just move above into this line:
int lp = currBox->y() + currBox->height();
I guess if you're going to rename "int lp" to "int bottomOfLastLinebox" then
the padding addition makes sense in the max line instead.

The if here is not needed:
if (firstLineBox()) {
for (InlineRunBox* currBox = firstLineBox(); currBox; currBox =
currBox->nextLineBox()) {

the for already has the if implicit in it.

The caret hack seems like it should be abstracted into some other place.  This
can't be the only place that wants paddingBoxWidthPlusCaret.  Is the caret
always 1px?


Happy to review the next round.


More information about the webkit-reviews mailing list