Bug 24999 - Optimize hit testing with transforms
Summary: Optimize hit testing with transforms
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: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-02 00:02 PDT by Simon Fraser (smfr)
Modified: 2009-04-02 09:48 PDT (History)
0 users

See Also:


Attachments
Testcase that calls elementFromPoint 200000 times (2.39 KB, text/html)
2009-04-02 00:03 PDT, Simon Fraser (smfr)
no flags Details
Patch, changelog (10.11 KB, patch)
2009-04-02 00:12 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Revised patch (10.17 KB, patch)
2009-04-02 09:11 PDT, Simon Fraser (smfr)
darin: 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) 2009-04-02 00:02:50 PDT
Hit testing with transforms goes through a more expensive code path in RenderLayer::hitTestLayer. This needs speeding up.
Comment 1 Simon Fraser (smfr) 2009-04-02 00:03:18 PDT
Created attachment 29184 [details]
Testcase that calls elementFromPoint 200000 times
Comment 2 Simon Fraser (smfr) 2009-04-02 00:12:46 PDT
Created attachment 29185 [details]
Patch, changelog

This patch, and the patch from bug 24648, take the runtime of this attached testcase from 1550ms to about 870ms.
Comment 3 Darin Adler 2009-04-02 07:44:57 PDT
Comment on attachment 29185 [details]
Patch, changelog

Our usual naming scheme is to use the word "get" in a function that uses an out parameter rather than a return value. Thus transformFromContainer would be renamed getTransformFromContainer.
Comment 4 Simon Fraser (smfr) 2009-04-02 09:11:10 PDT
Created attachment 29191 [details]
Revised patch
Comment 5 Darin Adler 2009-04-02 09:16:48 PDT
Comment on attachment 29191 [details]
Revised patch

> -    void move(int x, int y);
> +    void move(int x, int y);    // always flattens

The formatting here is slightly unconventional. We typically use a single space in cases like this rather than using, say, 4 spaces.

> +    void translate(int x, int y, bool accumulateTransform);

I am uncomfortable with boolean arguments in C++ functions. They're hard to read at the call site. The "true" doesn't help me understand what the function is doing. Further, the only caller of this function is passing true. Do we really need the boolean argument? If the version that flattens is needed, you could consider the "named enum" design pattern or having two separately named functions.

> +    void flattenWithTransform(const TransformationMatrix& t);

No need to include a name for the argument "t" here.

r=me
Comment 6 Simon Fraser (smfr) 2009-04-02 09:48:06 PDT
I used enums instead of bools, and addressed the other comments.
http://trac.webkit.org/changeset/42174