[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 bugzilla-daemon at webkit.org
Sun Jan 11 08:54:14 PST 2009

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

Attachment 26597: Fixes coord transformation for deeply nested frames

------- 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.

More information about the webkit-reviews mailing list