Bug 22215 – Avoid calling absoluteClippedOverflowRect() so many times

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

[Simon Fraser (smfr)]

There are a few places that call absoluteClippedOverflowRect(), then call repaintAfterLayoutIfNeeded() which computes the same rect again.

[Simon Fraser (smfr)]

Created attachment 25113 [details]
Patch

Avoid recomputing absoluteClippedOverflowRect() and absoluteOutlineBounds()
in  RenderObject::repaintAfterLayoutIfNeeded() if the caller has them to
hand and can pass them in.

[Simon Fraser (smfr)]

I did some PLT testing with this patch, but the impact is in the noise on my Mac Pro.

[Darin Adler]

Comment on attachment 25113 [details]
Patch

Comments on the patch, even though it's been withdrawn from review.

> +    // Repaint only if our old bounds and new bounds are different. bounds and outlineBox may be passed if known.
> +    bool repaintAfterLayoutIfNeeded(const IntRect& oldBounds, const IntRect& oldOutlineBox, IntRect* bounds = 0, IntRect* outlineBox = 0);

These should be "const IntRect*", since the recipient is being asked not to modify them.

> -        repaintAfterLayoutIfNeeded(oldBounds, oldOutlineBox);
> +        repaintAfterLayoutIfNeeded(oldBounds, oldOutlineBox);   // can we pass m_absoluteBounds?

I think this comment is a little too cryptic to check in: Who's asking the question and who would answer it? You could write a less cryptic comment, perhaps one that is a little more specific about why you're uncertain and what the benefit of passing it in would be. Maybe you can just leave it out for now. If you did want it in there, you should use our standard formatting, and use only one space before the command and capitalize the first word in the sentence.

> +        repaintAfterLayoutIfNeeded(oldBounds, oldOutlineBox);       // can we pass m_absoluteBounds?

Ditto.

[Simon Fraser (smfr)]

Created attachment 49160 [details]
Patch

Comment on attachment 49160 [details]
Patch

Debug builds could continue to compute the values and assert that they are equal to the passed-in precomputed values, if present. I also think that the “Rect” suffix isn’t a very clear way to distinguish between the arguments and the local variables (which are also rects!). A “Ptr” suffix or “precomputed” prefix might be clearer.

P.S. r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/55065