[webkit-reviews] review denied: [Bug 53258] TransformationMatrix::translateRight incorrectly computes a translateLeft. : [Attachment 80363] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has denied Shane Stephens
<shanestephens at google.com>'s request for review:
Bug 53258: TransformationMatrix::translateRight incorrectly computes a
translateLeft.
https://bugs.webkit.org/show_bug.cgi?id=53258

Attachment 80363: Patch
https://bugs.webkit.org/attachment.cgi?id=80363&action=review

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80363&action=review

>> Source/WebCore/rendering/TransformState.cpp:36
>> +		OwnPtr<TransformationMatrix> newVal =
OwnPtr<TransformationMatrix>(new TransformationMatrix());
> 
> It's a shame you have to allocate a new TransformationMatrix on the stack
here.

I don't understand what this code is doing. Why is the correct multiply by the
translation value on the left? If we're accumulating state, shouldn't the
translate be on the right? Is the caller expecting this odd behavior? It's hard
to search for 'move' (lots of classes have one). But maybe the call sites could
be changed to expect the opposite behavior. This could even be a case where the
call sites were accommodating the previous wrong behavoir. So maybe changing
both the caller and callee would get rid of a lot of this swizzling.

Seeing this makes me think we should comb the code more finely. There could be
many cases of "battling swizzles" that could make this hot code more efficient
in general.