Bug 22215 - Avoid calling absoluteClippedOverflowRect() so many times
Summary: Avoid calling absoluteClippedOverflowRect() so many times
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-12 15:11 PST by Simon Fraser (smfr)
Modified: 2010-02-21 13:47 PST (History)
0 users

See Also:


Attachments
Patch (6.88 KB, patch)
2008-11-12 18:21 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (4.49 KB, patch)
2010-02-21 12:02 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-11-12 15:11:55 PST
There are a few places that call absoluteClippedOverflowRect(), then call repaintAfterLayoutIfNeeded() which computes the same rect again.
Comment 1 Simon Fraser (smfr) 2008-11-12 18:21:21 PST
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.
Comment 2 Simon Fraser (smfr) 2008-11-12 18:22:00 PST
I did some PLT testing with this patch, but the impact is in the noise on my Mac Pro.
Comment 3 Darin Adler 2008-11-20 14:06:42 PST
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.
Comment 4 Simon Fraser (smfr) 2010-02-21 12:02:04 PST
Created attachment 49160 [details]
Patch
Comment 5 mitz 2010-02-21 12:09:55 PST
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
Comment 6 Simon Fraser (smfr) 2010-02-21 13:47:33 PST
http://trac.webkit.org/changeset/55065