Bug 102575 – [chromium] Add conversion between WebTransformation and gfx::Transform

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Add conversion between WebTransformation and gfx::Transform

[Dana Jansens]

Created attachment 174791 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 174791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review

Are you sure about adding these before we finish transitioning chromium code?  I'm worried about accidentally adding copies.

> Source/Platform/chromium/public/WebTransformationMatrix.h:160
> +        gfx::Transform t;

we don't use 1-letter variable names for anything but loop counters in WebKit

[Alexandre Elias]

Comment on attachment 174791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review

> Source/Platform/chromium/public/WebTransformationMatrix.h:147
> +                m_matrix[i][j] = t.matrix().getDouble(i, j);

One of these should go "j, i" because you need to transpose to convert between WebKit matrices and Skia matrices.

[Dana Jansens]

Created attachment 174793 [details]
Patch

[Dana Jansens]

Comment on attachment 174791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review

>> Source/Platform/chromium/public/WebTransformationMatrix.h:147
>> +                m_matrix[i][j] = t.matrix().getDouble(i, j);
> 
> One of these should go "j, i" because you need to transpose to convert between WebKit matrices and Skia matrices.

Oh snap, I thought I checked and it wasn't. Here's translateX

WebKit:
m_matrix[3][0]

Skia (via gfx):
matrix_.set(0, 3, SkFloatToScalar(x))

You're right. I misread! Will fix.

>> Source/Platform/chromium/public/WebTransformationMatrix.h:160
>> +        gfx::Transform t;
> 
> we don't use 1-letter variable names for anything but loop counters in WebKit

Oops, fixing.

[Dana Jansens]

Created attachment 174794 [details]
Patch

[Dana Jansens]

(In reply to comment #3)
> (From update of attachment 174791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review
> 
> Are you sure about adding these before we finish transitioning chromium code?  I'm worried about accidentally adding copies.

I am expecting the cc/ conversion to be ~1 patch, but it could be more than one, and then we should be careful. So, we can explicitly convert somewhere else, then delete it and add implicit conversions here. Or, we can have explicit ones here, and then make them implicit later.

I leave that to your discretion. I like them here because it's where the code is going to end up eventually anyways.

[James Robinson]

Comment on attachment 174794 [details]
Patch

I'd be fine with having these implicit if we can complete the conversion fast enough (1 patch would be ideal).  I don't want to get caught out in an intermediate state for too long.

R=me, up to you on the implicit/explicit

Comment on attachment 174794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174794&action=review

> Source/Platform/chromium/public/WebTransformationMatrix.h:148
> +                m_matrix[i][j] = transform.matrix().getDouble(j, i);

Maybe use SkMatrix44::asColMajord or asRowMajord?

[Dana Jansens]

(In reply to comment #9)
> (From update of attachment 174794 [details])
> I'd be fine with having these implicit if we can complete the conversion fast enough (1 patch would be ideal).  I don't want to get caught out in an intermediate state for too long.
> 
> R=me, up to you on the implicit/explicit

I do like being safe! And don't mind following up on this after. Thanks for the review.

[WebKit Review Bot]

Comment on attachment 174794 [details]
Patch

Attachment 174794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14857848

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Dana Jansens]

Comment on attachment 174794 [details]
Patch

Pretty sure this isn't crashing a test. Let's try this again.

[WebKit Review Bot]

Comment on attachment 174794 [details]
Patch

Clearing flags on attachment: 174794

Committed r135031: <http://trac.webkit.org/changeset/135031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.