[webkit-reviews] review denied: [Bug 20766] Unable to resize deeply nested frames : [Attachment 26597] Fixes coord transformation for deeply nested frames

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Treat <treat at kde.org>'s request
for review:
Bug 20766: Unable to resize deeply nested frames
https://bugs.webkit.org/show_bug.cgi?id=20766

Attachment 26597: Fixes coord transformation for deeply nested frames
https://bugs.webkit.org/attachment.cgi?id=26597&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great!

> +IntPoint RenderFrameSet::localPos(const IntPoint& point) const

I know that both xPos and yPos abbreviate the word "position" as "pos", but I'd
prefer to not add more uses of such abbreviations.

I also think that this converts a point from local, object-relative coordinates
to document coordinates. I don't think localPos is a good name for that. Maybe
Hyatt has a suggestion for a better name. We have functions named
localToAbsolute in RenderObject; maybe localToDocument? Or maybe you can use
localToAbsolute instead of writing a new function?

> +    IntPoint pos = IntPoint(point.x() + xPos(), point.y() + yPos());
> +    RenderObject* o = parent();
> +    if (!o || !o->isFrameSet())
> +	   return pos;
> +
> +    return static_cast<RenderFrameSet*>(o)->localPos(pos);

I'd prefer a looping algorithm to a recursive one. With recursive algorithms,
large data structures can become crashing bugs that might even have security
implications. When something is, like this, so easy to write as a loop, I'd
prefer to do so.

Bug fixes need regression tests. Please make a test demonstrating the bug and
the fact that it's fixed.