[webkit-reviews] review denied: [Bug 47514] CSS transforms should affect scrolling : [Attachment 70782] Patch to add topmostPosition()

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Beth Dakin <bdakin at apple.com>'s request
for review:
Bug 47514: CSS transforms should affect scrolling
https://bugs.webkit.org/show_bug.cgi?id=47514

Attachment 70782: Patch to add topmostPosition()
https://bugs.webkit.org/attachment.cgi?id=70782&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70782&action=review

Everything else looks good.  One more round.

> WebCore/rendering/RenderBlock.cpp:3385
> +		       int tp = r->y() + r->topmostPosition(false);

This check isn't quite right.  It needs to be like the one in lowestPosition. 
The comment is wrong too.

Should be something like this:

// If a positioned object lies completely to the left of the root it will be
unreachable via scrolling.
// Therefore we should not allow it to contribute to the topmost position.
if (!isRenderView() || r->x() + r->width() > 0 || r->x() +
r->rightmostPosition(false) > 0) {

> WebCore/rendering/RenderBlock.cpp:3416
> +	   top = min(top, relativeOffset - paddingTop());

Don't include "- paddingTop()" here. Just top = min(top, relativeOffset);

> WebCore/rendering/RenderBlock.cpp:3419
> +		   int childTopEdge = lastRootBox()->selectionTop();

You should be using firstRootBox() here not lastRootBox().

> WebCore/rendering/RenderBlock.cpp:3421
> +	       }

No need for the "- paddingTop()" here.

> WebCore/rendering/RenderBlock.cpp:3430
> +    }

I don't think this else check was needed at all.  Sorry for leading you astray
there.

I think this whole check should just reduce to:

if (!includeSelf && firstRootBox())
     top = min(top, firstRootBox()->selectionTop() + relativeOffset);

There may be more I'm missing, but that should be good enough for now.