[webkit-reviews] review granted: [Bug 24999] Optimize hit testing with transforms : [Attachment 29191] Revised patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 24999: Optimize hit testing with transforms
https://bugs.webkit.org/show_bug.cgi?id=24999

Attachment 29191: Revised patch
https://bugs.webkit.org/attachment.cgi?id=29191&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    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