Bug 24999 – Optimize hit testing with transforms

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

[Simon Fraser (smfr)]

Hit testing with transforms goes through a more expensive code path in RenderLayer::hitTestLayer. This needs speeding up.

[Simon Fraser (smfr)]

Created attachment 29184 [details]
Testcase that calls elementFromPoint 200000 times

[Simon Fraser (smfr)]

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.

[Darin Adler]

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.

[Simon Fraser (smfr)]

Created attachment 29191 [details]
Revised patch

[Darin Adler]

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

[Simon Fraser (smfr)]

I used enums instead of bools, and addressed the other comments.
http://trac.webkit.org/changeset/42174